-
Notifications
You must be signed in to change notification settings - Fork 87
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
Workflow - Detect dependency changes #1933
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1933 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 265 265
Lines 21712 21712
=======================================
Hits 21706 21706
Misses 6 6 Continue to review full report at Codecov.
|
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.
@ParthivNaresh looks good!
I see we're dumping the diff into the job output instead of saving it as an artifact. Fine by me.
One test I'd recommend you run before we merge, if you haven't already:
- Put up a separate draft PR with a branch based off this one
- On that branch, edit
latest_dependency_versions.txt
such that the deps update check should fail - Verify that it fails
Sound good?
.github/workflows/detect_changes.yml
Outdated
export DEPENDENCY_FILE_PATH=/tmp/dependencies_updated_artifacts/current_dependencies.txt | ||
evalml/tests/dependency_update_check/make_deps_diff.sh | ||
diff evalml/tests/dependency_update_check/latest_dependency_versions.txt /tmp/dependencies_updated_artifacts/current_dependencies.txt > /tmp/dependencies_updated_artifacts/diff.txt | ||
exit $? |
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.
@ParthivNaresh will GH actions error out if this error code is nonzero? That was the case with circleci, just confirming that'll work for GH too. If not, we should change this to make sure we get an error when there is a non-empty diff.
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.
Here's the result of the updated yaml:
CI Workflow: https://app.circleci.com/pipelines/github/alteryx/evalml/10515/workflows/014d87b3-38cb-4ee1-83b2-c7885c68f52a/jobs/120417
GitHub Action: https://github.com/alteryx/evalml/runs/2058345037?check_suite_focus=true
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!
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.
Looks good to me
Fixes #1926