Move Scaffolder API to OpenAPI#27771
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
|
Thanks for the contribution! |
a04277c to
0cd8958
Compare
Rugvip
left a comment
There was a problem hiding this comment.
Thank you for looking at this! 👍
Is the goal to wire up the router etc with this as well in this PR? I'd really be needed to ensure that the schema is correct and remains so.
| type: object | ||
| properties: | ||
| entity: | ||
| $ref: '../../../catalog-backend/src/schema/openapi.yaml#/components/schemas/UserEntity' |
There was a problem hiding this comment.
To be honest I think it's best to keep this one opaque. As an adopter of Backstage you can extend the UserEntity schema anyway, so there's kinda no point aiming to model this imo.
There was a problem hiding this comment.
Fair point. I'll adjust here, and remove the addition from the Catalog OpenAPI schema.
solimant
left a comment
There was a problem hiding this comment.
Is the goal to wire up the router etc with this as well in this PR? I'd really be needed to ensure that the schema is correct and remains so.
@Rugvip - Yes, that's the goal, and I'm working on making more commits for that. And, yes, the schema needs to be correct 100%. That's the reason why I started with that since everything else is pretty much a matter of code generation.
| type: object | ||
| properties: | ||
| entity: | ||
| $ref: '../../../catalog-backend/src/schema/openapi.yaml#/components/schemas/UserEntity' |
There was a problem hiding this comment.
Fair point. I'll adjust here, and remove the addition from the Catalog OpenAPI schema.
0cd8958 to
93cc245
Compare
|
@aramissennyeydd - I'm cleaning up a few things, and I noticed that we've got some schemas defined in OpenAPI and the corresponding generated TypeScript type, and also defined separately (manually) in other places. For example, |
|
@benjdlambert - On a separate note, I'm seeing an issue with properties that have special characters, such as |
|
@Rugvip - because I've come across a few Scaffolder types that use |
Yup this is intentional, the duplication here is minimal and the OpenAPI version should be looser than the catalog one.
Similarly here, I don't think we gain much if we deduplicate the types and could start running into issues where we expect x version of the shared types and get y.
I can take a look into a fix here - in the meantime, can you just add these as an |
| description: Step that is part of a Template Entity. | ||
| TemplateEntityV1beta3: | ||
| allOf: | ||
| - $ref: '../../../catalog-backend/src/schema/openapi.yaml#/components/schemas/Entity' |
There was a problem hiding this comment.
Thank you very much for the response, @aramissennyeydd. Would like to get your feedback on this as well. Do we wanna cross-reference types from other OpenAPI specs like this one? For this one, the TypeScript type I'm matching is defined here, and is used in the Scaffolder router here.
There was a problem hiding this comment.
nope - just copy this over from the other spec. I haven't figured out the stitching between specs yet. This kind of route may fail if you were you install this package on your local instance.
|
@aramissennyeydd - for the Scaffolder client, I'm thinking to do the following:
What do you think? Or should we create a new package called |
|
I should've clarified the part about |
|
@solimant The
|
|
This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution! |
|
Please remove the "stale" label. |
|
Hi @solimant, the best way to avoid this going stale while you wait for reviews is to rebase. Your branch is currently 690 commits behind. |
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
Signed-off-by: solimant <solimant@users.noreply.github.com>
benjdlambert
left a comment
There was a problem hiding this comment.
Just some minor bits and then we can ship I think.
| @@ -0,0 +1,10 @@ | |||
| --- | |||
| '@backstage/backend-openapi-utils': minor | |||
There was a problem hiding this comment.
| '@backstage/backend-openapi-utils': minor |
This should probably be in another changeset, and also only a patch
There was a problem hiding this comment.
There's also a changeset needed for repo-tool as well?
| | 'failed' | ||
| | 'open' | ||
| | 'processing'; | ||
| | 'processing' |
There was a problem hiding this comment.
Yeah, think these types might need a cleanup, but happy to take that before the mainline release if that's OK? Going to try and ship this ASAP.
| if (taskId) { | ||
| analytics.captureEvent('retried', 'Template has been retried'); | ||
| await scaffolderApi.retry?.(taskId); | ||
| await scaffolderApi.retry?.({ taskId }); |
There was a problem hiding this comment.
I think that this is a breaking change to the client which we can't make yet, this needs updating in our wrapping client to take just taskId.
There's a bigger piece of work which is to make sure that all the functions of the scaffolder API follow this pattern ({ taskId }) for example, but rather make that change as a bigger breaking change instead.
Signed-off-by: benjdlambert <ben@blam.sh>
Signed-off-by: benjdlambert <ben@blam.sh>
Signed-off-by: benjdlambert <ben@blam.sh>
|
@solimant I created a separate changeset for the I think that covers everything for now, but if you're willing to cleanup the types in a followup PR those deprecations will need to also be called out explicitly in another changeset too 🙏 Approving and auto-merge on! Thanks a lot for the effort here all round from everyone involved 🎉 @aramissennyeydd too 👍 |
Signed-off-by: benjdlambert <ben@blam.sh>
|
Thank you for contributing to Backstage! The changes in this pull request will be part of the |
Hey, I just made a Pull Request!
Closes #23819
Server and client generated using:
✔️ Checklist
Signed-off-byline in the message. (more info)