-
Notifications
You must be signed in to change notification settings - Fork 292
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
[Docs] CI for variables.yaml + CI for docs update #2457
base: master
Are you sure you want to change the base?
Conversation
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.
Nice! Some nits though.
- name: Run YAML Lint | ||
run: | | ||
pip install yamllint==1.35.1 | ||
yamllint flow/scripts/variables.yaml |
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.
Can we pick a yaml linting tool that, like black, for python, you can just run to fix the formatting?
I'm not picky on yaml linting rules, I just want automatic update, like black does for python.
else | ||
echo "has_update=false" >> "$GITHUB_OUTPUT" | ||
fi | ||
git add . |
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.
this can add anything in the source folder, why add anything beyond FlowVariables.md?
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.
In fact, if anything under source control is change, except FlowVariables.md, this action should terminate, no?
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.
nits
Also, I think it is worth creating a prior pull request that only updates the formatting of variables.yaml so that this PR does not update the variables.yaml, that way you avoid pull request merge conflicts if this PR needs to be hold off.
name: ORFS variables.yaml tester and linter | ||
|
||
on: | ||
push: |
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.
shouldn't this be pull-request, not on push(to a branch) test linting?
otherwise I don't think it will tests pull requests as only those with write access to the ORFS can create branches
The rest of the PR I think is designed to run after updates to master
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.
If we are catching mismatches on pull-request then I wouldn't expect we would ever need to have the docs-pr-update. The issue would be fixed in the original PR prior to merging.
Signed-off-by: Jack Luar <[email protected]>
…ction Signed-off-by: Jack Luar <[email protected]>
b64ab9e
to
e26f811
Compare
Signed-off-by: Jack Luar <[email protected]>
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.
Suggestion: If FlowVariables.md is modified and it does not match the generatedocs.py result in a PR fail the PR.
Signed-off-by: Jack Luar <[email protected]>
@oharboe FlowVariables.md changes are detected in the update-pr section. Do you mean to shift it up to the lint section? |
I am thinking this is more linting than testing, yes. It is fast enough.... |
yamlfix.toml
Outdated
@@ -0,0 +1,11 @@ | |||
explicit_start = true | |||
line_length = 120 |
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 looks like it is being wrapped at 80 characters not 120. 80 is preferable as it match Google style for c++. Does this do anything?
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.
Yes, it does. The command to run for check is yamlfix -c yamlfix.toml flow/scripts/variables.yaml --check
, whereas for format is yamlfix -c yamlfix.toml flow/scripts/variables.yaml
. Next PR should reflect line length 80.
Signed-off-by: Jack Luar <[email protected]>
Signed-off-by: Jack Luar <[email protected]>
Tracked in #2452
Fixes #2444