-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(api): ensure StackRunConfig #3543
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
base: main
Are you sure you want to change the base?
Conversation
682fdef
to
9353e4b
Compare
working on this -- might convert to live in |
59a098a
to
90a9e9c
Compare
uh oh, pre commit seems broken still? |
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.
Overall, I like the approach. I have a few questions:
- how do we intend to use this? Do we bump the version each time a new change to the schema is introduced?
- How do we refresh the
tests/api/snapshots/test_models/test_run_config_v1_schema_is_unchanged/stored_run_config_v1_schema.json
file? Should pre-commit do it? Should we run the scripts/api-conformance.sh script with a special flag to rehydrate?
I think we need some guidelines, not just a script that fails the CI :) (unless I missed something!)
Thanks!
@@ -0,0 +1,25 @@ | |||
#!/bin/bash |
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.
can we use a more accurate name for the script?
2c3c461
to
f694a21
Compare
@leseb , I made it so that when a breaking change is acknowledged through the proper process Also changed the name of the script to |
2c65dee
to
0cacb3d
Compare
FYI I need to rebase this each time the run config changes, sorry for the churn :) |
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.
can you remove tests/api/snapshots/test_models/test_run_config_v1_schema_is_unchanged/stored_run_config_v1_schema.json
?
oasdiff breaking --fail-on ERR $BASE_SPEC $CURRENT_SPEC --match-path '^/v1/' | ||
# never skip this, instead if a breaking change is properly identified -- we should regenerate our snapshot. | ||
- name: Run Pydantic Model Test |
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.
why don't we run this in pre-commit?
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.
I think keeping it in the conformance yml makes sense though so it can get the context of the !
or BREAKING CHANGE:
from this workflow. In pre-commit people will just override the snapshots locally and repush.
scripts/api-conformance.sh
Outdated
pytest -s -v "$SCRIPT" --snapshot-update | ||
# When regenerating, always exit 0 to make the test pass | ||
exit 0 | ||
else | ||
pytest -s -v "$SCRIPT" --snapshot-update |
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.
same command regardlesss if REGENERATE_SNAPSHOT
is set?
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.
made this:
# Run pytest with snapshot testing
echo "=== Running Snapshot Tests ==="
if [[ "$REGENERATE_SNAPSHOT" == "true" ]]; then
echo "Regenerating snapshots..."
pytest -s -v "$SCRIPT" --snapshot-update
exit 0
else
pytest -s -v "$SCRIPT" # do not update snapshots.
fi
so that the test only updates the snapshots and passes if the proper ack is given. The exit 0 is necessary because even when updating snapshots these types of tests fail on the diff I think.
the other way will fail if the snapshots differ.
cf7abc7
to
b86766d
Compare
@@ -0,0 +1,368 @@ | |||
name: PR Bot Commands |
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 is a re-written version of precommit-trigger. I used git mv
but because the diff is so large it's registering it as a new file annoyingly. Sorry
b62a28a
to
680cfde
Compare
moving to draft while I wrestle with the new action I am introducing. |
680cfde
to
8dd9656
Compare
StackRunConfig is part of our public API, ensure stability of this datatype using a pytest snapshot test. If the pydantic model changes, it will fail. A snapshot can be re-generated via `@github-actions regenerate snapshots` by a code owner. The API conformance test will then re-run and pass. Signed-off-by: Charlie Doern <[email protected]>
8dd9656
to
af94606
Compare
What does this PR do?
StackRunConfig is part of our public API, ensure stability of this datatype using a pytest snapshot test.
If the pydantic model changes, it will fail. A snapshot can be re-generated via
@github-actions regenerate snapshots
by a code owner.The API conformance test will then re-run and pass.
added
json_schema_extra
to two spots in the run config so user paths do not get propagated into the snapshot jsonTest Plan
API Conformance Test should pass.