pants icon indicating copy to clipboard operation
pants copied to clipboard

Add OpenAPI backend

Open jyggen opened this issue 2 years ago • 4 comments

This PR aims to add an initial backend for OpenAPI documents as outlined in #16042, most thoughts behind the PR is there but here's a quick rundown:

  • openapi_definition and openapi_source targets (and openapi_definitions and openapi_sources)
    • openapi_definition is for the main OpenAPI document and openapi_source is also for that file as well as for any JSON/YAML file referenced by it (and files referenced by those files and so on)
    • this was done since most tooling only cares about the main document and will resolve references themselves, and then it's easier for rules to simply target openapi_definition instead of having to go through all openapi_source and guess the entry points
  • dependency inference for openapi_definition is simply checking if there is a openapi_source target on the same file
  • dependency inference for openapi_source is done by following all $ref in the file that points to local files
  • tailor will first find all openapi.json or openapi.yaml files not owned and create openapi_definitions in those dirs, then do dependency inference on those files and create openapi_sources for unowned files (as well as for the openapi.json or openapi.yaml files)

There's probably some tweaking in naming and behaviour that needs to be done based on discussions (either here or in #16042). The difficult part is that OpenAPI documents are simply JSON and YAML files so, unlike most other backends, you can't assume that all files with a certain extension is relevant, which is especially tricky with tailor.

jyggen avatar Jul 16 '22 22:07 jyggen

As I already mentioned in the issue ticket, I love the idea of having this backend, just not fully convinced we need two different target types.

alonsodomin avatar Jul 20 '22 15:07 alonsodomin

I left a comment in the issue: Could the primary openapi target be something other than openapi_definition? That overloads the meaning of "definition". Maybe one of these?

  • openapi_document / openapi_documents
  • openapi_doc / openapi_docs
  • openapi_spec / openapi_specs

I'm fine with openapi_source for the secondary targets.

cognifloyd avatar Jul 27 '22 19:07 cognifloyd

In some cases, tools need to know which spec version to expect. For instance, I can imagine a codegen that takes a v2 spec and transforms it to v3 or vice versa.

I wonder if we can "infer" the spec version in addition to the dependencies. Or if it needs a field users can explicitly set the version on. I guess the spec version is kind of like interpreter_constraints...

Good point, though I don't think I personally have ever had to tell a tool what version of the spec it is. Usually they infer it from the version property in the document itself, and I think that's probably the easiest way to go about it if this backend ever wants to know the spec version.

jyggen avatar Jul 27 '22 23:07 jyggen

Thanks for changing the target name. That feels better to me.

This PR is focused on plumbing the targets and dep inference, right? So, linting will come in follow-up PRs?

That's correct. Some basic linting will follow 👌

jyggen avatar Jul 27 '22 23:07 jyggen

@jyggen sorry for not merging this earlier, we'd like to include this in the future 2.15 release, would you mind rebasing this with main as there have some changes in the formatters since this has been initially submitted.

Thanks.

alonsodomin avatar Sep 08 '22 17:09 alonsodomin

@jyggen sorry for not merging this earlier, we'd like to include this in the future 2.15 release, would you mind rebasing this with main as there have some changes in the formatters since this has been initially submitted.

I've just rebased. Thanks for the reminder!

stuhood avatar Sep 08 '22 17:09 stuhood

Thanks!

El jue, 8 sept, 2022 a las 19:39, Stu Hood @.***> escribió:

@.***(https://github.com/jyggen) sorry for not merging this earlier, we'd like to include this in the future 2.15 release, would you mind rebasing this with main as there have some changes in the formatters since this has been initially submitted.

I've just rebased. Thanks for the reminder!

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>

alonsodomin avatar Sep 08 '22 17:09 alonsodomin

@jyggen: Note that this doesn't add the backend to src/python/pants/bin:plugins, so it won't yet actually go in the pantsbuild.pants dist. If you're ready for that, would you mind opening another PR to do that?

stuhood avatar Sep 08 '22 17:09 stuhood

@stuhood Sure, I can do that. Thanks!

jyggen avatar Sep 08 '22 17:09 jyggen

@stuhood A BUILD file needed some formatting after the rebase, so I pushed a fix for that.

jyggen avatar Sep 08 '22 18:09 jyggen