-
Notifications
You must be signed in to change notification settings - Fork 18
Add xml to yaml converter, update Markdown and Yaml files automatically when pull requests are merged #117
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
Conversation
YAML; still todo: comment formatting
…into feature/xml2yaml
| @@ -1,4 +1,4 @@ | |||
| # Earth System Modeling Standard Name Library | |||
| # Earth System Modeling Standard Name Library - add some chunk here to test auto update | |||
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.
Ignore this - to prove that the auto update works. The file will be re-rendered from the XML and submitted to the repository when this PR is merged.
| @@ -0,0 +1,5689 @@ | |||
| library_name: Earth System Modeling Standard Name Library - add some chunk here to test auto update | |||
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.
Ignore this ("- add some chunk here to test auto update") - to prove that the auto update works. The file will be re-rendered from the XML and submitted to the repository when this PR is merged.
| if stdn_longname is None: | ||
| sdict = {'standard_name':stdn_name} | ||
| stdn_longname = standard_name_to_long_name(sdict) | ||
| # Step through the sections |
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 changes to parse_section other than adjusting the indentation to 4 whitespaces
gold2718
left a comment
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.
One question and not the most thorough review but this looks okay to me.
.github/workflows/commit_to_main.yml
Outdated
| echo "The following changes will be committed (git diff Metadata-standard-names.yaml):" | ||
| git diff Metadata-standard-names.yaml | ||
| git add Metadata-standard-names.yaml | ||
| git commit -m "Update Metadata-standard-names.yaml from standard_names.xml" || echo "No changes to commit" |
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.
It seems likely that if the markdown is unchanged then YAML is also unhanged. Why not write both files, then run git?
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.
True. A little bit of reorg, but happy to make the change.
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.
Done in 9097066
nusbaume
left a comment
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, but did have a couple questions and one (optional) request.
…ack in, add release/* to branches
nusbaume
left a comment
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 great to me now. Thanks!
dustinswales
left a comment
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.
@climbfuji This is really slick!
|
@mkavulich Shall we go ahead and merge? If we run into trouble with the github bot not being able to commit the updated xml and yamls, then I can look into alternative options before the next meeting. |
|
@climbfuji It occurs to me that we can ensure the actions will work before merging by adding the "workflow_dispatch" option and kicking off the file creation manually; is there any downside to including this manual option for the new GitHub action? |
The code lives in a branch in my fork and you can't trigger a workflow from a branch in a different fork. |
|
Thanks for the explanation. I will go ahead and merge then, and keep an eye on the result. |
|
@climbfuji The workflow is failing due to branch protection rules that prevent direct pushes to main. It appears as if there are options available that do not involve removing branch protections, so I am looking into those. |
Thanks @mkavulich . Happy to work with you on that. I was thinking that we could also add the commit to the PR action itself, but trigger it only when a label is added. If we commit the updated files every time a developer pushes a commit, the developers have to pull the updates into their local clone (annoying). The downside of the label solution is that we have to remember adding the label to run the action before we merge. But maybe a big and fat check box at the top of the PR template could help with that. |
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.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.
Add xml to yaml converter, update Markdown and Yaml files automatically when pull requests are merged (ESCOMP#117) This pull request adds the capability to render the XML standard names dictionary to Yaml, in addition to the existing rendering to Markdown. I decided to render comments into long strings that the Python Yaml library then wraps correctly. I validated the resulting Yaml file using https://yamlchecker.com and it is valid. This pull request further updates the existing GitHub action for consistency and standard Yaml formatting (2-space indentation), and replaces the check if the Markdown is updated with tests to render the xml to both md and yaml. Lastly, a second GitHub workflow is added that automatically renders the xml to md and yaml and commits the files to the repository. This action is only run when the authoritative branch (main) is updated (push = merge a pull request). I tested this action in my fork of ESM standard names (see https://github.com/climbfuji/CCPPStandardNames/actions/runs/15637497788/job/44056347886). What I did not test is how this works with branch protection (my main branch was not protected). We will find out if additional settings/changes are needed once this PR is approved and merged. Closes ESCOMP#113
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.
Description
This pull request adds the capability to render the XML standard names dictionary to Yaml, in addition to the existing rendering to Markdown. I decided to render comments into long strings that the Python Yaml library then wraps correctly. I validated the resulting Yaml file using https://yamlchecker.com and it is valid.
This pull request further updates the existing GitHub action for consistency and standard Yaml formatting (2-space indentation), and replaces the check if the Markdown is updated with tests to render the xml to both md and yaml.
Lastly, a second GitHub workflow is added that automatically renders the xml to md and yaml and commits the files to the repository. This action is only run when the authoritative branch (
main) is updated (push= merge a pull request). I tested this action in my fork of ESM standard names (see https://github.com/climbfuji/CCPPStandardNames/actions/runs/15637497788/job/44056347886). What I did not test is how this works with branch protection (my main branch was not protected). We will find out if additional settings/changes are needed once this PR is approved and merged.Closes #113