-
Notifications
You must be signed in to change notification settings - Fork 57
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
Adding basic pre-commit to the repo #440
Conversation
Should we also add Any test that is not enforced by the CI will get broken eventually i think :) |
4abfc0b
to
f5974c4
Compare
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
PR has expanded considerably due to the CI integration. But I've minimized changes to actually executable code by skipping bash checks for all ".bash" files. It seems to me that at least some of them are just templates. |
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.
Thanks, i noticed two spots where this accidentally reverts recent changes, but other than that LGTM
xref:comparing-configuration-files-between-deployments_storage-requirements[Comparing configuration files between deployments]. | ||
Make sure you have pull the os-diff repository and configure according to your environment: | ||
link:planning.md#Configuration tooling[Configure os-diff] | ||
//kgilliga: Should we use this link in the downstream guide? |
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 is reverting a recent fix
@@ -130,7 +130,3 @@ ${BASH_ALIASES[openstack]} server list | grep -qF '| test | SHUTOFF |' && ${BASH | |||
${BASH_ALIASES[openstack]} server list | grep -F '| test | ACTIVE |' && \ | |||
${BASH_ALIASES[openstack]} server --os-compute-api-version 2.48 show --diagnostics test --fit-width -f json | jq -r '.state' | grep running || echo FAIL | |||
---- | |||
|
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.
Ditto this is a revert
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's not visible in the "discussion" view on the PR but if you open the file detail it will be visible what it's removing.
Checks include basic style and ansible-lint Ansible-lint entry point includes override for the ANSIBLE_ROLES_PATH var, to ensure repo only roles load at runtime. Bash checks on files matching ".*.bash" are disabled as they are often not scripts but rather templates. Linting was applied to all other files. Github action was simplified so that from now on pre-commit takes care of everythin. Signed-off-by: Jiri Podivin <[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.
👍 Thank you
Checks include basic style and ansible-lint
Ansible-lint entry point includes override for the ANSIBLE_ROLES_PATH var, to ensure repo only roles load at runtime.