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 GitHub Actions CI for pull requests #34

Merged
merged 4 commits into from
May 15, 2023
Merged

Add GitHub Actions CI for pull requests #34

merged 4 commits into from
May 15, 2023

Conversation

jfrost-mo
Copy link
Contributor

@jfrost-mo jfrost-mo commented Apr 27, 2023

Fixes #33

Also has a second actions job to check that the markdown rendering of the standard names is up to date. It would be fairly easy to extend this to add a commit to the PR updating it if it isn't, but I haven't added that here as it wasn't really the focus.

Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for opening this PR! I didn't even realize the Met Office was interested in this project, but we'll happily take any help we can get.

Anyways, I tried testing the Actions workflow and found that one step related to xmllint was missing for each job. However, once those changes are made and the merge conflict is resolved I think this PR will be good to go, at least on my end (i.e. the workflow tests all behaved as expected in my testing environment). Of course If you need any assistance with the requested changes just let me know.

.github/workflows/pull_request_ci.yml Show resolved Hide resolved
.github/workflows/pull_request_ci.yml Show resolved Hide resolved
@jfrost-mo jfrost-mo requested a review from nusbaume April 28, 2023 07:48
@jfrost-mo
Copy link
Contributor Author

jfrost-mo commented Apr 28, 2023

I've added xmllint installation to the workflow, and rebased onto main to fix the merge conflicts.
As context, my interest in this project stems from its use in JEDI.

Copy link
Collaborator

@nusbaume nusbaume left a 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 now, thanks again!

Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for adding this capability, @jfrost-mo !

@mkavulich mkavulich merged commit a81b4ca into ESCOMP:main May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add check_xml_unique Github Action
4 participants