test: introduce api conformance test #3257
Conversation
meta-cla
bot
added
the
CLA Signed
|
need to write my own GH action for the openapi check, the action I am referencing is kind of out of date. |
cdoern
force-pushed
the
conformance
branch
13 times, most recently
from
32608b9 to
0e1913d
Compare
cdoern
requested review from
ashwinb,
yanxi0830,
hardikjshah,
raghotham,
ehhuang,
terrytangyuan,
leseb,
bbrowning,
reluctantfuturist,
mattf and
slekkala1
as code owners
|
is there a need for docker here? maybe we can a more lightweight approach like oasdiff in python and we can have a script that can also do path filtering instead of using command line tools directly? Also, if its lightweight enough, we can maybe make it be part of pre-commit? |
Sure! I can try using oasdiff. The custom containerfile element kind of snuck in due to the yq weirdness, I agree avoiding spinning up a container might be better. I'll see if I can rework this. |
.github/workflows/conformance.yml
Outdated
| ref: ${{ github.event.pull_request.base.ref }} | ||
| path: 'base' | ||
|
|
||
| # Build a custom container that includes both openapi-diff and yq tools |
There was a problem hiding this comment.
nit: there is no longer a container?
There was a problem hiding this comment.
sorry @raghotham , need to push one or two more changes to cleanup and change the description!
| run: | | ||
| oasdiff breaking --fail-on ERR base/docs/_static/llama-stack-spec.yaml docs/_static/llama-stack-spec.yaml --match-path '^/v1/openai/v1' \ | ||
| --match-path '^/v1/vector-io' \ | ||
| --match-path '^/v1/vector-dbs' |
There was a problem hiding this comment.
what is the recourse when there is a legitimate breaking change?
There was a problem hiding this comment.
I was thinking maintainers could apply a label that would override the failing test. Makes it such that these changes need to be very purposeful
There was a problem hiding this comment.
this likely should be a different PR made by a maintainer so it can be tested and such, right?
There was a problem hiding this comment.
I was thinking something like combine with #3260 where if the PR that is breaking compatibility has a PR title prefix. Having a prefix would turn the oasdiff to just warning rather than failing
There was a problem hiding this comment.
sure, lets merge #3260 first perhaps and then I can adapt it to run this as well? or vice-versa, either way is fine with me
this test runs on each PR and uses a new conformance workflow to compare the base (main) branch openapi spec to the one on this PR if one of our "stable" APIs change, the test will fail. this workflow uses `oasdiff` to identify breaking changes for paths we want to ensure comptability for. specifically this is using `oasdiff breaking` with `--match-path` which only checks breaking changes for the specified paths. As a follow up to this, we can add an optional way to make it so that it is OK to make these change if properly documented or in a changelog or something. related to llamastack#3237 Signed-off-by: Charlie Doern <[email protected]>
What does this PR do?
this test runs on each PR and uses a new conformance workflow to compare the base (main) branch openapi spec to the one on this PR if one of our "stable" APIs change, the test will fail.
this workflow uses
oasdiffto identify breaking changes for paths we want to ensure comptability for.specifically this is using
oasdiff breakingwith--match-pathwhich only checks breaking changes for the specified paths.As a follow up to this, we can add an optional way to make it so that it is OK to make these change if properly documented or in a changelog or something. or by using a label on the PR to override the failing test.
related to #3237
Test Plan
conformance test should pass given there are no changes