Skip to content
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

Add PR-Based Config Testing Workflow #120

Open
CodeGat opened this issue Sep 5, 2024 · 10 comments · Fixed by #121 · May be fixed by #122
Open

Add PR-Based Config Testing Workflow #120

CodeGat opened this issue Sep 5, 2024 · 10 comments · Fixed by #121 · May be fixed by #122
Assignees
Labels
CI Continuous integration infrastructure priority:high

Comments

@CodeGat
Copy link
Contributor

CodeGat commented Sep 5, 2024

In a similar vein to the existing access-om2-configs/access-esm1.5-configs CI infrastructure, create an access-om3-specific PR-based workflow. Work with @anton-seaice to develop requirements - they could be different to the above config repositories.

In any case, use the generic access-nri/model-config-tests reusable workflows that already exist (may have to be modified to support new requirements), to allow for easy porting of features/fixes to other config repositories. This doesn't mean that the CI infrastructure that is specific to this repository (i.e., the stuff in $this_repo/.github/workflows that isn't calls to generic workflows) has to remain the same.

@CodeGat CodeGat added the CI Continuous integration infrastructure label Sep 5, 2024
@CodeGat CodeGat self-assigned this Sep 5, 2024
@anton-seaice
Copy link
Contributor

We are planning to rename our branches to follow the same naming format as other repositories, e.g. with release-, dev- and feature- prefixes to standardize with the other workflows.

We would like on creation/update of a PR, for it to trigger the workflow run and then have a mechanism for the developer to confirm they want the checksum to be updated if they change (e.g. a trigger word for the bot, which then commits the new checksum to the branch).

We would also like to capture the four files from the output with the file names MOM_parameter_doc.* (as artifacts), and commit them to the branch under test. (ping @minghangli-uni - is this correct?)

Its probably also worth rerunning the workflow after a merge commit to confrim the merge hasn't changed anything.

The main thing initially is to get the workflow running, and over time, we can improve and add tests to the model-config-tests repo.

@CodeGat
Copy link
Contributor Author

CodeGat commented Sep 10, 2024

I linked the wrong issue, whoops

@CodeGat CodeGat reopened this Sep 10, 2024
@CodeGat
Copy link
Contributor Author

CodeGat commented Sep 11, 2024

Thanks @anton-seaice, getting to this now.
Note that the feature- branches don't have to literally have feature- on them - they can be any branch.
All those features you want can be supported - although the merge commit part really shouldn't change the answers since it's a requirement that the branch be rebased onto the HEAD of release-, but it can be worked in if you want it.

@anton-seaice
Copy link
Contributor

@minghangli-uni Just confirming, we want to copy archive/output000/MOM_parameter_doc.* to the docs directory in the config? (Is that the right files?)

@aekiss - Do we want the test to fail if the files have changed, or commit them? (Or fail and the files can be committed with a keyword action in a comment(e.g. !update_docs)

@anton-seaice
Copy link
Contributor

anton-seaice commented Sep 11, 2024

All those features you want can be supported - although the merge commit part really shouldn't change the answers since it's a requirement that the branch be rebased onto the HEAD of release-, but it can be worked in if you want it.

Ok - so we need to confirm that is the setting in the repository settings ?

edit: I guess I mean, if you click Rebase and Merge, that might change the answer right ? Because the checksum would have been from before the Rebase ?

@CodeGat
Copy link
Contributor Author

CodeGat commented Sep 11, 2024

There is a branch protection setting that enforces that merging is only available if your are on the HEAD of the PR target branch

@anton-seaice
Copy link
Contributor

@aekiss
Copy link
Contributor

aekiss commented Sep 11, 2024

Linking to issue discussing MOM_parameter_doc.*: COSIMA/access-om3#117

@minghangli-uni Just confirming, we want to copy archive/output000/MOM_parameter_doc.* to the docs directory in the config? (Is that the right files?)

Sounds good to me. We could put them in the root directory, but using a docs dir in each config makes it clear they aren't input files. Ideally payu would also copy the most recent archive/output*/MOM_parameter_doc.* to the docs directory after each run so that the git runlog would record any changes.

@aekiss - Do we want the test to fail if the files have changed, or commit them? (Or fail and the files can be committed with a keyword action in a comment(e.g. !update_docs)

I think fail and the files can be committed with a keyword action in a comment (e.g. !update_docs)

@CodeGat
Copy link
Contributor Author

CodeGat commented Sep 12, 2024

Thanks for the clarification @aekiss

In terms of the case where the files in docs/ change, the workflow gives a big 🚫 and you're given a comment that says "docs/ was modified in this run, commit these using !update_docs" - under which circumstance would you not do !update_docs? The current state would mean that the PR isn't mergable in this state because of the 🚫 (which is resolved by !update_docs).

@anton-seaice
Copy link
Contributor

Thanks - that sounds correct @CodeGat

You would only not update docs if it the PR had some issue with it (e.g. changing defaults from a new binary that weren't expected) that is addressed in some other way (e.g. by changing the config).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration infrastructure priority:high
Projects
None yet
3 participants