New procedure for updating .md and .yaml files #122
Conversation
…an be run manually as the user desires before merge
…thor or code manager
mkavulich
requested review from
cacraigucar,
climbfuji,
dustinswales,
gold2718,
grantfirl,
mattldawson,
mwaxmonsky,
nusbaume,
peverwhee,
ss421 and
svahl991
as code owners
mkavulich
changed the title
mkavulich
changed the title
mkavulich
changed the title
|
I prefer a slightly different solution, but it goes in the same direction. Let's chat in an hour |
|
The solution I had in mind yesterday is in #123, but there is one problem with it - the pull request branch can't come from a fork because of permissions, it must be in the base repository. This is not what we want. There are insecure workarounds for it that I don't think we should use ( |
| # Checks if the saved markdown matches freshly rendered markdown. | ||
| # If this fails, prompt user to update | ||
| checksum=$(sha256sum Metadata-standard-names.md) | ||
| tools/write_standard_name_table.py --output-format md standard_names.xml | ||
| echo "The following changes will be committed when this pull request is merged (git diff Metadata-standard-names.md; " | ||
| echo "assuming that 'Metadata-standard-names.md' wasn't updated and matches the version in the authoritative branch):" | ||
| git diff Metadata-standard-names.md | ||
| test "$checksum" = "$(sha256sum Metadata-standard-names.md)" || exit "Markdown file Metadata-standard-names.md must be updated; see documentation for details" |
There was a problem hiding this comment.
Why not just run git diff after line 78 and check the return code?
Generally, why do we need this. I would revert the changes to pull_request_ci.yml entirely. Can't we just add a checkbox to the pull request template that instructs the code managers to check if the rendered files need to be updated, run the workflow_dispatch action and then tick the box? See #123
There was a problem hiding this comment.
I'm happy removing these actions all together and returning to the manual updates of old. It would be nice to have an automatic (or at least push-button) solution to update these tables within GitHub but it could be getting that working seems to be more trouble than it's worth.
There was a problem hiding this comment.
I wonder, yes. Maybe the best approach is a pragmatic one. Your PR has it so that the checks fail because the files must be updated. If we add a big bold message there (and in the documentation) that tells the user to update and commit the files, and how to do it, then that is perfectly fine for me.
| push: | ||
| branches: | ||
| - main | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Do you know if this works? How does this change circumvent the branch protection that the original logic doesn't?
There was a problem hiding this comment.
If the commit is done on the PR branch prior to merge rather than in main then there are no branch protection issues (I think). But this would require that the user's fork has been updated to include this workflow file already, since workflow_dispatch tasks must be in that specific repository's main branch.
…ather than checksum
…lude some more help for installing needed python package
|
@climbfuji I have removed the automated task and updated the instructions for users to update the MD/YAML files when the test fails. Let me know if you think we need more. |
|
Excellent, thanks! I see you tested this in https://github.com/ESCOMP/ESMStandardNames/actions/runs/17280713958/job/49048314628. |
New procedure for updating .md and .yaml files (ESCOMP#122) Because it is a bit of a hassle for PR authors to have to make changes to multiple files, ESCOMP#117 attempted to automate the process using the `tools/write_standard_name_table.py` tool. However, this new GitHub action could not run automatically due to branch protection rules on the `main` branch. Rather than remove those restrictions, this PR reverts to the old method of manual updates, but provides more extensive documentation and user prompts to do this updating properly.
3 participants
Description
Because it is a bit of a hassle for PR authors to have to make changes to multiple files, #117 attempted to automate the process using the
tools/write_standard_name_table.pytool. However, this new GitHub action could not run automatically due to branch protection rules on themainbranch. Rather than remove those restrictions, this PR proposes a compromise with a GitHub action that can optionally be run by the user or code manager to update these files, while also allowing the user to do this process manually if desired. This way, the changes will be made prior to merge, and so the test that prevents merge if the .md and/or .yaml files have not been updated has been re-implemented.Issues
Addresses #121