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

Validate OpenAPI specification in CI #481

Merged
merged 7 commits into from Aug 31, 2020
Merged

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Aug 31, 2020

This PR adds an invoke task to get the response from https://validator.swagger.io/debug for a given URL. The CI then uses the commit SHA/repository to construct the URL for our OpenAPI schema, currently hard-coded to be at openapi/openapi.json and openapi/index_openapi.json, which it then pings the validator with.

@ml-evs ml-evs force-pushed the ml-evs/ci_openapi_validator branch from 66fe896 to 99d148b Compare August 31, 2020 15:09
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not many comments right now. As we're discussing on Slack as well, I'll just recap a bit:

  • Possibly use POST instead to send the latest generated openapi.json as well as index_openapi.json content.
  • Add the badge to the README (possibly).

In addition, I think we should remove openapi-spec-validator as a dependency and all uses of it.

tasks.py Outdated Show resolved Hide resolved
@ml-evs ml-evs force-pushed the ml-evs/ci_openapi_validator branch from 0c560c0 to feb0eeb Compare August 31, 2020 15:22
@ml-evs
Copy link
Member Author

ml-evs commented Aug 31, 2020

In addition, I think we should remove openapi-spec-validator as a dependency and all uses of it.

Agreed, I've just replaced the openapi CI task with this.

tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 31, 2020

Codecov Report

Merging #481 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #481   +/-   ##
=======================================
  Coverage   91.67%   91.67%           
=======================================
  Files          60       60           
  Lines        2848     2848           
=======================================
  Hits         2611     2611           
  Misses        237      237           
Flag Coverage Δ
#unittests 91.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c70050e...90559d2. Read the comment docs.

@CasperWA
Copy link
Member

In addition, I think we should remove openapi-spec-validator as a dependency and all uses of it.

Agreed, I've just replaced the openapi CI task with this.

openapi-spec-validator should also be removed from requirements-dev.txt.

Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>
@ml-evs ml-evs force-pushed the ml-evs/ci_openapi_validator branch from dfbb8d7 to 5695cb4 Compare August 31, 2020 15:53
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great now. A final simplification from my side.

tasks.py Outdated Show resolved Hide resolved
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
@ml-evs
Copy link
Member Author

ml-evs commented Aug 31, 2020

RE: readme badge, would you want it to just point to the master version of openapi/openapi.json? I'm not sure I'm that fussed...

@CasperWA
Copy link
Member

RE: readme badge, would you want it to just point to the master version of openapi/openapi.json? I'm not sure I'm that fussed...

Yeah, that was my idea.
If you find it looks terrible, we'll just leave it out.

@ml-evs
Copy link
Member Author

ml-evs commented Aug 31, 2020

Yeah, that was my idea.
If you find it looks terrible, we'll just leave it out.

There's a shields.io version we can use:

img

Will add it later on once the other PRs are in.

Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great with one less (direct) dependency, thanks @ml-evs !

@ml-evs ml-evs merged commit 3d4b5bc into master Aug 31, 2020
@ml-evs ml-evs deleted the ml-evs/ci_openapi_validator branch August 31, 2020 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants