-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
feat: add support for validating OpenAPI specs against test runs #19955
Conversation
4b658ca
to
16c9945
Compare
Changed Packages
|
Uffizzi Preview |
An example of a failed run output, https://github.com/backstage/backstage/actions/runs/6226398906/job/16899164878 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Very nice to get this output in ci 👏
I took a peak and added some questions!
a17b6ba
to
a7ee771
Compare
Looks like we run into some OOM errors in CI 😬
|
@jhaals I think the GC error is due to running the test command in a sub shell? As is, I think we're running 25 node workers. I can adjust this down to 1 for CI as I wasn't hitting this locally. |
b5691a1
to
73988c4
Compare
4c62856
to
f1cd7bd
Compare
@Rugvip This should be ready for a re-review when you get a chance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, some rough review comments
schemas: | ||
Error: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mimics ErrorResponseBody
right? If so, it needs some double checking I think. The request field is not required, and the error field has optional stack
and code
. Check the same in the catalog one of course too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also raises an interesting question about sharing common schemas across packages, since this is such a central type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yup, will update to match that type.
I've been thinking about the shared common schemas for a while, but kicking it down the road. Entities were the one I was primarily thinking about, but this should also be added to the stack. Interested to hear any ideas here, haven't been able to come up with a solution as yet.
return execPromise( | ||
[ | ||
command, | ||
...options.filter(e => e).map(e => (e.startsWith('-') ? e : `"${e}"`)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, manual quoting is a bit unfortunate. Can this be rewritten to use the spawn('a', ['b', 'c'], { windowsVerbatimArguments: true }
form or so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on using the CLI's run command here? I think it would require extracting into cli-node
or cli-common
, but would come with other benefits like stdout forwarding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for the files belonging to catalog
@sennyeya Seems the builds got real confused here ... things have been improved a lot lately, you may want to rebase (maybe squash too? see if the changeset warning clears up) and force push to kick the tires a bit, until a review gets in |
618e70a
to
c1574fa
Compare
@freben It should be updated now, the yarn file conflicts are going to be the death of me... |
600d652
to
8ecf8d6
Compare
Thanks for the contribution! |
bf981ff
to
dc82ef9
Compare
return; | ||
} | ||
|
||
const opticLocation = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freben When you get a chance, one thing that I've been thinking about on top of this one is where to install optic. I think repo-tools
is the best place for it, so had to add this to get the binary location to then run in the context of other directories. Hopefully, there's a better way to do this...
one kind of weird thing with Optic is they derive the config file based on context and you can't override it with a passed in value, opticdev/optic#2256. Ideally, we could just run optic
in repo-tools
and point to each directory, but for now that's a bit of a pipe dream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we could do is leave optic
as a peer dependency and require the target repo to install it through the root package.json
instead. That'd skip the installation for anyone that doesn't use it as well.
Another option to the way it's run is if optic expose a JS API as well that allows us to run these equivalent command straight through JS instead. A bit like we do with Jest here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍, more or less good to go imo, just need to get deps in order. Rest I think can be future improvements
return; | ||
} | ||
|
||
const opticLocation = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we could do is leave optic
as a peer dependency and require the target repo to install it through the root package.json
instead. That'd skip the installation for anyone that doesn't use it as well.
Another option to the way it's run is if optic expose a JS API as well that allows us to run these equivalent command straight through JS instead. A bit like we do with Jest here
… OpenAPI schema feedback. Signed-off-by: Aramis Sennyey <aramiss@spotify.com> Signed-off-by: Aramis <sennyeyaramis@gmail.com>
Signed-off-by: Aramis <sennyeyaramis@gmail.com> Signed-off-by: Aramis Sennyey <aramiss@spotify.com>
Signed-off-by: Aramis Sennyey <aramiss@spotify.com>
1497991
to
2191bb3
Compare
Signed-off-by: Aramis Sennyey <aramiss@spotify.com>
@Rugvip I can try and get Optic to update their set up upstream, but currently they only support using it through the CLI. Would we want it to be installed as a dependency per your comment on the size of the dependency tree? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 👍
Let's
Thank you for contributing to Backstage! The changes in this pull request will be part of the |
Hey, I just made a Pull Request!
Optic has a command called
optic capture
that supports making requests through a proxy that can be validated against the written OpenAPI spec afterwards. It also supports a--update
flag for automated updates. Wanting to add this intocatalog-backend
andsearch-backend
as an initial test.✔️ Checklist
Signed-off-by
line in the message. (more info)