-
Notifications
You must be signed in to change notification settings - Fork 91
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
Reorganization of the GHA files #2801
Conversation
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.
@bmhan12, @rrsettgast if you are fine with this I would just go ahead and merge it.
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.
No more final job to find if everything went OK; "gate" jobs allow to enter conditional jobs instead of using if in yaml which make the job optional (?).
Awesome! I just want to make sure: in the case where CUDA jobs are not run ('ci: ready to be merged' is not set), does the workflow show an expected failure status?
I found that without the convoluted "check_that_all_jobs_succeeded" job, a workflow will pass and the PR is mergeable without having ran the CUDA tests.
Co-authored-by: Brian Han <han12@llnl.gov>
Hi @bmhan12 and thanks for the review. |
Hi @bmhan12 I've just merged the PR. I did notice that the |
The integrated tests will be run as part of the workflow when the label "ci: run integrated tests" is associated to the PR. A bundle of the new reference files and of the logs is sent to the cloud and made available for download to the developer. (For the sake of merging, this step deactivated in this PR.). For the moment, integrated tests are only requested to be run on 2 procs. This can be changed easily. Important note: while the integrated tests can be run, they are not passing yet. This is why the integrated tests feature is deactivated. We'll manage this in a follow-up PR. Also: -The GHA yaml files now only manage input data and job dependencies. In the end, the job informations are later provided to ci_build_and_test_in_container.sh directlyScript ci_build_and_test.sh, which was previously calling ci_build_and_test_in_container.sh could be removed. -The number of environment variables in the GHA workflow is reduced to nearly the maximum. (LA variables remain because I was afraid of conflicts. This can be done in a follow-up PR if wanted.) The scripts now use getopt instead. - No duplicated call to the scripts for all the builds. One does all the cases with an decent number of if. -Initial job is_pull_request_a_draft provides all the output requested from the PR (assignees, draft, ...). It uses a single rest call and uses the GHA output pattern. - No more final job to find if everything went OK; "gate" jobs allow to enter conditional jobs instead of using if in yaml which make the job optional (?). - The GEOS_TPL_TAG is not defined in .github/workflows/ci_tests.yml anymore. It's defined in and read from .devcontainer/devcontainer.json to avoid duplication. Documentation is updated accordingly. - Extracting the data (geos executable bundle or integrated tests results) was simplified and do not require docker cp anymore. The bundling process does not need to have access to out-of-scope information (like the commit or the PR number). - The credentials for sccache is simplified a bit: no need for a specific mount point and copy of secrets. codespaces can now write in the GEOS-DEV submodules of the GEOS repository (in particular integratedTests). - Fixes brittle assumptions when building the ats run scripts (in Fix paths when creating ats run scripts ntegratedTests#58)
The integrated tests can be run as part of the workflow when the label
ci: run integrated tests
is associated to the PR.A bundle of the new reference files and of the logs is sent to the cloud and made available for download to the developer. (For the sake of merging, this step deactivated in this PR.).
For the moment, integrated tests are only requested to be run on 2 procs. This can be changed easily.
Important note: while the integrated tests can be run, they are not passing:
Also:
yaml
files now only manage input data and job dependencies.In the end, the job informations are later provided to
ci_build_and_test_in_container.sh
directly.Script
ci_build_and_test.sh
, which was previously callingci_build_and_test_in_container.sh
could be removed.(
LA
variables remain because I was afraid of conflicts. This can be done in a follow-up PR if wanted.)The scripts now use
getopt
instead.if
.is_pull_request_a_draft
provides all the output requested from the PR (assignees
,draft
, ...).It uses a single rest call and uses the GHA output pattern.
if
inyaml
which make the job optional (?).GEOS_TPL_TAG
is not defined in.github/workflows/ci_tests.yml
anymore. It's defined in and read from.devcontainer/devcontainer.json
to avoid duplication. Documentation is updated accordingly.geos
executable bundle or integrated tests results) was simplified and do not requiredocker cp
anymore.The bundling process does not need to have access to out-of-scope information (like the commit or the PR number).
sccache
is simplified a bit: no need for a specific mount point and copy of secrets.codespaces
can now write in theGEOS-DEV
submodules of theGEOS
repository (in particularintegratedTests
).Relates to issue #2800