More complete discovery of entry point playbooks #5028
Conversation
|
@mtnbikenc, Looking through this code I see that you tied into the syntax check. Was there a reason why you chose this and not creating a separate check? I think for efficiency sake, we are already traversing the files, and the logic is quite similar so I think this is probably fine. If we want to expound or change anything in the future we can break this out. |
setup.py
Outdated
| if not isinstance(task, dict): | ||
| # Skip yaml files which do not contain plays or includes | ||
| continue | ||
| if 'include' in task.keys(): |
There was a problem hiding this comment.
nit: keys is not needed.
if 'include' in task:There was a problem hiding this comment.
Same situation here as in hosts below, just being specific. This will also need to be extended once we move to Ansible 2.4 and start switching over to import_playbook.
setup.py
Outdated
| os.path.join(os.path.dirname(yaml_file), | ||
| included_file_name)) | ||
| included_playbooks.add(included_file) | ||
| elif 'hosts' in task.keys(): |
There was a problem hiding this comment.
nit: keys is not needed.
elif 'hosts' in task:There was a problem hiding this comment.
Actually, keys is needed. I ran into a case when the word hosts was merely present in the text of the dict as a whole. This generated a false positive, so I was more specific in looking at the keys of the dict and not the whole dict.
There was a problem hiding this comment.
I've not seen that before, but I believe you ... though I can't reproduce it.
Python 3
In [1]: a = {'a': 'b'}
In [2]: 'a' in a
Out[2]: True
In [3]: 'b' in a
Out[3]: False
Python 2
>>> a = {'a': 'b'}
>>> 'a' in a
True
>>> 'b' in a
False
>>>
In the end it's a nit though so 👍 😄
There was a problem hiding this comment.
I've unraveled the mystery. The problem was two fold and hidden by the first. What I was actually encountering was a difference in the type of object being read from the yaml file. In one instance (cluster_hosts.yml), I was presented with a str and was getting a positive on hosts in that str. When I ultimately added a check to skip the task if it was not a dict, I was able to bypass that false positive. So, I don't really need the keys as you suggested since I'm already verifying I'm working with a dict. Updated. 😃
| @@ -221,27 +221,43 @@ def run(self): | |||
| ''' run command ''' | |||
|
|
|||
| has_errors = False | |||
| playbooks = set() | |||
setup.py
Outdated
| has_errors = True | ||
| # Break for loop, no need to continue looping lines | ||
| break | ||
| for task in yaml.safe_load(contents.read()): |
There was a problem hiding this comment.
nit: I realize this wasn't something added in this PR but technically contents is a file instance. It reads a bit off to me to need to read the contents to get the contents of the instance.
|
@kwoodson The original syntax checking was finding playbooks to check only by the existence of 'initialize_hosts.yml` being in the playbook. This is not a reliable way of finding the actual playbooks to check. This new method finds all the playbooks that could be run and is not tied to just the inclusion of a particular file. I'm attempting to improve the completeness of our syntax checking by making this change. |
mtnbikenc
force-pushed
the
entry-point-validation
branch
from
9c348dc to
a68dcd6
Compare
This change scans for supported entry point playbooks based on the following conditions: * In the supported directory of playbooks/byo * Playbooks not included by any other playbooks
mtnbikenc
force-pushed
the
entry-point-validation
branch
from
a68dcd6 to
3c7d816
Compare
|
aos-ci-test |
|
aos-ci-test |
|
[merge] |
|
[test]ing while waiting on the merge queue |
|
Evaluated for openshift ansible test up to 3c7d816 |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_openshift_ansible/427/) (Base Commit: 6528031) (PR Branch Commit: 3c7d816) |
|
Flake: openshift/origin#14898 yum: no mirrors to try |
|
[merge] |
|
flake again on yum: openshift/origin#8571 |
|
[merge] |
|
Evaluated for openshift ansible merge up to 3c7d816 |
|
continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/833/) (Base Commit: 7c7b91c) (PR Branch Commit: 3c7d816) |
This change scans for supported entry point playbooks based on the following conditions:
The previous method of discovering entry point playbooks by searching files for
intialize_hosts.ymlwas incomplete and would only find properly formatted playbooks. This method finds all playbooks, as well as new playbooks, which are not included by other playbooks.Note: Some playbooks which may be a documented entry point, such as byo/config.yml, are not directly identified by this method because of higher up playbooks which also include those playbooks.
Test affected: