-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 Ansible syntax checks to tox #3938
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.
I'd like to see "ERROR" prefix that line, but I won't stop this from moving forward if you don't want to.
setup.py
Outdated
print('Execution failed: %s' % cpe) | ||
has_errors = True | ||
if has_errors: | ||
print('Ansible syntax-check issues found') |
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.
I ran this test (python setup.py ansible_syntax
) and got the error message above ^
nit: Ansible syntax-check issues found
is a pretty mundane looking output message. Could we prefix that with ERROR: or something? I almost missed it (though, there were obvious errors printed in the messages above that lint).
I won't block on it though.
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.
I'm fine with dropping that message altogether. It is not really providing much value. I'd rather have the lines with Execution failed
printed in red. Whenever I'm looking through Travis logs, I scroll down looking for red. The only thing that is reported in red for the syntax check is the final InvocationError:
, then you have to scroll back up to see all the individual errors.
@tbielawa Made a few changes regarding the output style. Colorized the individual error messages and dropped the less useful print statements. |
@mtnbikenc looks great. I was thinking red too. Nice touch. 👍 |
aos-ci-test |
[merge] |
Evaluated for openshift ansible merge up to b1898ac |
[test]ing while waiting on the merge queue |
Evaluated for openshift ansible test up to b1898ac |
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_openshift_ansible/56/) (Base Commit: 9ace041) |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/259/) (Base Commit: b0b66e5) |
Adds syntax checking for entry point playbooks to standard tox checks run by CI. An entry point playbook is identified as being a .yml/.yaml file containing
initialize_groups.yml
.This PR will fail Travis CI until #3936 and #3937 are merged.