-
Notifications
You must be signed in to change notification settings - Fork 1
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
Make Repro-CI Artifacts Reflect Configs Repo Structure #74
Conversation
… to mimic default repo structure
…artifact to mimic default repo structure
…/testing` directory for checksums
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 accidentally just commented directly, but I have a few questions.
…or a custom directory structure Moved all `additional-artifact` logic into it's own job
…t to `test-repro`
Alright, there's been a bit of change since the last review @aidanheerdegen - this is because I realised that in
|
Closing this for now as it deals with committing |
* Add config-comment-repro.yml workflow for repro tests on any branch! * Replace `against COMMITISH` with `compare COMMITISH` * Added ability to commit the result of the repro test to the PR * Added RUN_URL, removed some of the branch name stuff * Add additional artifact content paths section for repro-ci * Removed custom `[compare COMMITISH]` logic * Update artifact structure to be in line with #74 * Renamed file, made it a more generic `!test` command * Changed artifact paths back to initial version, removed `inputs.additional-artifact-content-paths` input * Removed issue property from `github.event.comment.body` * Reworked permission checking, bug fixes, comment updates * Further changes from testing * Added error-checking for `!test` command * Made comments more informative * Update README.md to account for new `!test repro` syntax * Update .github/workflows/config-comment-test.yml Co-authored-by: Aidan Heerdegen <[email protected]> * README.md: Note the case-sensitivity of the command * Add step in prepare-command to check the command starts with `!test` --------- Co-authored-by: Aidan Heerdegen <[email protected]>
The Problem
Currently, the struture of the checksums in a repro test experiment running on Gadi are the following:
And the structure of the checksums in the repo are:
In which, in the CI, we special-case the logic for creating the intermediate
testing
directory in the repo. This also creates problems for theinputs.additional-artifact-content-paths
, which are, confusingly, in the correct place at both the experiment level and the repo level, which means they both have to be handled differently.The Solution
This PR unifies the checksum directories so that both the experiment and repo reflect the following structure:
In this PR:
./testing
directory in artifact and experiment to mimic default repo structure./testing
directory in artifact and experiment to mimic default repo structure./testing
directory for checksumsNOTE: There is a unhealthy amount of duplication of effort in the PR, especially in
test-repro
/generate-checksums
(see 34b7697 and 37518f4). This will be rectified in #72.NOTE: This is required to merge #73
Closes #75