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

RHDHPAI-29: Add yaml formatting validation #41

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jrichter1
Copy link

What does this PR do?:

Adds simple yaml formatter linting to check PRs/commits, along with fixes that the formatter generated.

Which issue(s) this PR fixes:[

https://issues.redhat.com/browse/RHDHPAI-29

How to test changes / Special notes to the reviewer:

The CI action removes the any conditionals for the yamlfmt run. For local testing, I recommend commenting such lines.

Copy link
Contributor

@thepetk thepetk left a comment

Choose a reason for hiding this comment

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

In general the updates look good to me! left only a minor comment to pin a dependency.

I'd suggest testing the updated code with software templates.

QQ: Do you have a test run of the action (for example in your fork)?

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@jrichter1
Copy link
Author

QQ: Do you have a test run of the action (for example in your fork)?

I did a run https://github.com/jrichter1/ai-lab-app/actions/runs/12807711620/job/35708874921, it doesn't really show much though, since there are no issues found.

@thepetk
Copy link
Contributor

thepetk commented Jan 16, 2025

QQ: Do you have a test run of the action (for example in your fork)?

I did a run https://github.com/jrichter1/ai-lab-app/actions/runs/12807711620/job/35708874921, it doesn't really show much though, since there are no issues found.

Nice! Then I think just a quick test with the software templates to see that everything works fine. Mostly because we update a large number of files.

Co-authored-by: Theofanis Petkos <[email protected]>
@jrichter1
Copy link
Author

I've just run the template, and some of the variable substitution does not like being wrapped in single quotes, or rather the results might make for an invalid yaml when the value is not a string. Time to try without them...

@jrichter1
Copy link
Author

In this form, the template works properly for me (feel free to not take my word for it though)

Copy link
Contributor

@johnmcollier johnmcollier 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 from my end.

Some of the rearranging done by the yaml formatter had me worried that the nunjucks conditionals would get broken, but I compared them with what we have in main, and everything was preserved.

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.

3 participants