Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🚀 Feature: introduce symmetry to schema openapi generate-* commands #21790

Closed
2 tasks done
taras opened this issue Dec 8, 2023 · 10 comments
Closed
2 tasks done

🚀 Feature: introduce symmetry to schema openapi generate-* commands #21790

taras opened this issue Dec 8, 2023 · 10 comments
Labels
enhancement New feature or request

Comments

@taras
Copy link
Member

taras commented Dec 8, 2023

🔖 Feature description

backstage-repo-tools schema openapi generate command operates on the entire repository and doesn't take any arguments. backstage-repo-tools schema openapi generate-client expects a path to openapi schema and the root of the workspace where the code will be generated. This setup works fine if the development of the backend plugin starts before the frontend client. It's awkward if that work happens in parallel. It's not a significant issue, but making it feel more consistent would be nice.

🎤 Context

I had the pleasure of showing the new OpenAPI generator functionality to developers in our workshop, and it was very well received. Updating the schema and re-generating the client felt very natural. I wanted to share some of the bumpy parts of the process so we could iron them out.

✌️ Possible Implementation

One possibility is to have a package generate command added to @backstage/cli that would invoke generate on every workspace that defines the script. In the workspace, we could specify backstage-repo-tools schema openapi generate client --input= --output= or backstage-repo-tools schema openapi generate server --input= ---output=.

👀 Have you spent some time to check if this feature request has been raised before?

  • I checked and didn't find similar issue

🏢 Have you read the Code of Conduct?

Are you willing to submit PR?

Yes I am willing to submit a PR!

@taras taras added the enhancement New feature or request label Dec 8, 2023
@sennyeya
Copy link
Contributor

sennyeya commented Dec 8, 2023

Thanks for this! Couple of questions on the bumpiness and then I'll respond to the concerns.

This setup works fine if the development of the backend plugin starts before the frontend client. It's awkward if that work happens in parallel. It's not a significant issue, but making it feel more consistent would be nice.

  1. Would this be helped if the generate-client command generated a full client package instead of just a generated folder? How much of DefaultApi.client.ts were you using?

This setup works fine if the development of the backend plugin starts before the frontend client.

  1. Did you start with an OpenAPI schema? Or were you working iteratively across the full frontend/backend? I was working with Andre last week to try and migrate the Azure DevOps plugin to the OpenAPI tooling and found some issues on the iteration.

The generate-client command ended up being a little different than the existing generate command because we had to support the weirdness of the packages/catalog-client setup. I could definitely see some sane defaults being added though, like specifying a package instead of a path to a schema file and having the output directory default to plugins/{pluginId}-client with an option to override.

re: an overall generate command, I spoke with @Rugvip about something like this at KubeCon and I think it would be great. An interim step between now and then could be migrating the existing generate command to generate server and generate-client to generate client. A similar structure could be nice for the verify command as well for CI/CD. Alternatively, we could put these under a named argument on the generate command to make it a little less hectic.I like the idea of a package vs repo distinction as it's not super clear from the command itself which one does which. I could see generate being just package and verify being both.

I'm not completely sold that this should sit in @backstage/cli, as it's a bit more advanced and pretty use case specific than a lot of the commands there. It also adds some more versioning complexity that sounds painful.

@taras
Copy link
Member Author

taras commented Dec 8, 2023

Would this be helped if the generate-client command generated a full client package instead of just a generated folder?

Not necessarily. I think that use case could be covered under backstage-cli new but the fact that it only generates needed files allows us to re-run the same generator multiple times when openapi.yaml updates.

How much of DefaultApi.client.ts were you using?

All of it.

Did you start with an OpenAPI schema?

In this case, we were creating a full-stack plugin from scratch. We started with the UI, created an OpenAPI schema for it, implemented the UI, generated types from the schema, and created the ApiRef, hooked it up to the component.

Or were you working iteratively across the full frontend/backend?

We did a bit of iterating. We updated the schema halfway through, then re-ran the generator and updated the typescript to get it to compile.

I was working with Andre last week to try and migrate the Azure DevOps plugin to the OpenAPI tooling and found some issues on the iteration.

What were the issues?

I'm not completely sold that this should sit in @backstage/cli, as it's a bit more advanced and pretty use case specific than a lot of the commands there. It also adds some more versioning complexity that sounds painful.

Yeah, we don't need to worry about this right now. It might make sense to add generate to @backstage/cli in combination with caching - maybe Turbo Repo?

@sennyeya
Copy link
Contributor

sennyeya commented Dec 8, 2023

... that use case could be covered under backstage-cli new

Oh yeah, true.

How much of DefaultApi.client.ts were you using?
All of it.

Ah sorry, my question here was were you using DefaultApi.client.ts as the raw exported client? Or did you add a wrapper like in CatalogClient.ts?

In this case, we were creating a full-stack plugin from scratch. We started with the UI, created an OpenAPI schema for it, implemented the UI, generated types from the schema, and created the ApiRef, hooked it up to the component.

Cool! 🚀

What were the issues?

Mostly on the "generating a spec from test data" side. The whole flow is pretty bumpy and not something I tested out with the catalog when I was starting out. There are a couple of easy improvements and some longer term issues with how we're executing the subcommands.

It might make sense to add generate to @backstage/cli in combination with caching - maybe Turbo Repo?

Definitely a question for the core maintainers. If this tooling starts getting more traction, the caching is going to be really important.

@taras
Copy link
Member Author

taras commented Dec 8, 2023

were you using DefaultApi.client.ts as the raw exported client? Or did you add a wrapper like in CatalogClient.ts?

We were using the export directly. We didn't wrap it. The API Factory instantiated the class explored from the generated code.

Mostly on the "generating a spec from test data" side.

Oh, I see. Hmm... I didn't try this at all. Is this using Optic?

@sennyeya
Copy link
Contributor

sennyeya commented Dec 8, 2023

We were using the export directly. We didn't wrap it.

Oooo, cool, thoughts on the API? One object with body, query, path as typed subobjects.

Oh, I see. Hmm... I didn't try this at all. Is this using Optic?

Yup, it's more for migrating/bootstrapping existing plugins to the tooling by generating a basic spec from test data sent locally using supertest. Really cool stuff.

@taras
Copy link
Member Author

taras commented Dec 8, 2023

thoughts on the API?

It's a bit weird but not too weird. Where does that format come from? I assumed it was OpenAPI Tools. Without looking at the generated types, I tried to pass path & query together, but then types told me otherwise. I would expect something like getProducts(params, options).

@sennyeya
Copy link
Contributor

sennyeya commented Dec 8, 2023

Where does that format come from?

From conversation on the PR, I think mostly around API stability. My previous iteration was getProducs(id, params, body, options) which wasn't great if things changed. Combining path and query seems odd to me, how would you handle name conflicts and whatnot.

@taras
Copy link
Member Author

taras commented Dec 8, 2023

Combining path and query seems odd to me, how would you handle name conflicts and whatnot.

I assumed that conflicts are not possible. Does OpenAPI allow params with same names in path and query?

@sennyeya
Copy link
Contributor

sennyeya commented Dec 8, 2023

I assumed that conflicts are not possible. Does OpenAPI allow params with same names in path and query?

Unfortunately yes, it's probably the weakest part of OpenAPI. Parameters are just an array of objects with an in: ... block. At the spec level, there's nothing stopping you from defining the same parameter twice with different types, which is not great. Tooling might prevent that, but it's a hard requirement to pin down especially across the ecosystem.

@taras
Copy link
Member Author

taras commented Dec 8, 2023

From looking at this, it seems like treacherous territory to combine them. We'd probably eventually regret this decision so let's forget that.

sennyeya added a commit that referenced this issue Jan 18, 2024
fixes: #21790

Signed-off-by: Aramis <sennyeyaramis@gmail.com>
@Rugvip Rugvip closed this as completed in ff3a604 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants