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

[CI] Add step that runs backward compatibility tests on PRs #1592

Merged

Conversation

Tankilevitch
Copy link
Contributor

@Tankilevitch Tankilevitch commented Dec 29, 2021

https://jira.iguazeng.com/browse/ML-1274

A few tools have been examined:

  1. https://github.com/OpenAPITools/openapi-diff
  2. https://github.com/Tufin/oasdiff
  3. https://www.npmjs.com/package/openapi-diff

Since we are currently using 3.0.1 of OpenApi with FastApi, there are a few differences in the capabilities that JSON schema provides that are only available in the 3.1.0 version of OpenApi.

That is further explained at tiangolo/fastapi#240 (comment).

These distinctions have an impact on the usability of oasdiff and npm/openapi-diff.
Currently, openapi-diff is the only one capable of handling the aforementioned differences.

Important Notes

  • I was impressed by the tool’s explainability capabilities after resolving the OpenApi and JsonSchema differences in order to run oasdiff.
  • Although openapi-diff is useful, it does not provide much useful information about the exact cause of the API breakage, necessitating additional investigation to determine the exact cause. I propose that we switch to oasdiff once fastapi supports OpenApi 3.1.0.

@Tankilevitch Tankilevitch changed the title test [CI] Add API backward compatibility check Dec 30, 2021
Copy link
Contributor

@Hedingber Hedingber left a comment

Choose a reason for hiding this comment

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

Please add description to the PR:
What other tools you've checked, why you picked openapi-diff etc...

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
tests/api/api/test_docs.py Outdated Show resolved Hide resolved
tests/api/api/test_docs.py Show resolved Hide resolved
tests/api/api/test_docs.py Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@Hedingber Hedingber left a comment

Choose a reason for hiding this comment

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

Please note the previous review comment - add description
Also it seems like you're still writing the output of the docker run command to a file, so I unresolved #1592 (comment)

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@Hedingber Hedingber changed the title [CI] Add API backward compatibility check [CI] Add step that runs backward compatibility tests on PRs Jan 2, 2022
Copy link
Contributor

@Hedingber Hedingber left a comment

Choose a reason for hiding this comment

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

Very nice work!

@Hedingber Hedingber merged commit 9ad9565 into mlrun:development Jan 2, 2022
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