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.

Sign up for GitHub

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

Jump to bottom

Allow tree-ish versions for ansible-galaxy #13203

Merged
merged 2 commits into from
Feb 17, 2016
Merged

Allow tree-ish versions for ansible-galaxy #13203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9761250
Allow tree-ish to be used for galaxy role version
semifocused Sep 27, 2015
ff19233
Add tests for #10620
willthames Oct 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions lib/ansible/galaxy/role.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,5 +310,3 @@ def spec(self):
}
"""
return dict(scm=self.scm, src=self.src, version=self.version, name=self.name)


11 changes: 11 additions & 0 deletions lib/ansible/playbook/role/requirement.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,17 @@ def scm_archive_role(src, scm='git', name=None, version='HEAD'):
if rc != 0:
raise AnsibleError ("- command %s failed in directory %s (rc=%s)" % (' '.join(clone_cmd), tempdir, rc))

if scm == 'git' and version:
checkout_cmd = [scm, 'checkout', version]
with open('/dev/null', 'w') as devnull:
try:
popen = subprocess.Popen(checkout_cmd, cwd=os.path.join(tempdir, name), stdout=devnull, stderr=devnull)
except (IOError, OSError):
raise AnsibleError("error executing: %s" % " ".join(checkout_cmd))
rc = popen.wait()
if rc != 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a reason do not use a simple test?
imho, it's more zen :)

if rc:
   ...

Copy link
Contributor

Choose a reason for hiding this comment

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

The approach in the modification regarding testing rc was "adopt-and-go" with existing coding conventions. Shifting those conventions for enhanced zen may be desired, but the scope of doing so would be larger than this PR. Considering occurences in the ansible code base of rc != 0:

~/git/ansible$ find . -type f | xargs -I FILE grep "rc != 0" FILE /dev/null | wc -l
353

Making this change may or may not be desired, if so might it best be undertaken in its own PR?

Copy link
Member

Choose a reason for hiding this comment

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

i'm fine with the less 'zenliness' of rc != 0 as this reflects the non pythonic shelling out to a command line utility

raise AnsibleError("- command %s failed in directory %s (rc=%s)" % (' '.join(checkout_cmd), tempdir, rc))
Copy link
Member

Choose a reason for hiding this comment

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

it wold be helpful to get the error from the scm, but i would worry then that the wait call might block, it would require changing the code to account for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very similar (to the extent it might be worth abstracting it) code is in role/requirement.py in scm_archive_role twice. If it's good enough for git clone and git archive, surely it's good enough for git checkout.

Copy link
Member

Choose a reason for hiding this comment

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

i've been meaning to fix that also, i finally let it go cause I did not have the time, i spent too much trying to restructure galaxy and roles. This is not a showstopper.


temp_file = tempfile.NamedTemporaryFile(delete=False, suffix='.tar')
if scm == 'hg':
archive_cmd = ['hg', 'archive', '--prefix', "%s/" % name]
Expand Down
2 changes: 1 addition & 1 deletion test/integration/galaxy_roles.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
name: oracle_java7

- src: git+http://bitbucket.org/willthames/git-ansible-galaxy
version: v1.6
version: pr-10620

- src: http://bitbucket.org/willthames/hg-ansible-galaxy
scm: hg
Expand Down
2 changes: 1 addition & 1 deletion test/integration/galaxy_rolesfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# deliberate non-empty whitespace line to follow


git+https://bitbucket.org/willthames/git-ansible-galaxy,v1.6
git+https://bitbucket.org/willthames/git-ansible-galaxy,pr-10620
hg+https://bitbucket.org/willthames/hg-ansible-galaxy
https://bitbucket.org/willthames/http-ansible-galaxy/get/master.tar.gz,,http-role
# comment
Expand Down