galaxy: Handle hidden git directories in role skeleton #72035

manas-init · 2020-09-30T20:59:18Z

ansible-galaxy role init --role-skeleton=ansible-role-skeleton example fails fix.
The issue was because 'dir' variable was changed to reference to a different list, but since it is a global variable so the update was causing issues.
Now updating 'dir' variable to change value of variable in local scope.

fixes #71977

SUMMARY

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

ADDITIONAL INFORMATION

bcoca

please add a test (so we don't revert again) and a changelog so we can backport this.

manas-init · 2020-10-01T20:21:16Z

please add a test (so we don't revert again) and a changelog so we can backport this.

Hi @bcoca I'm new to this. Could you please guide me on how to do the above mentioned.

manas-init · 2020-10-01T20:57:28Z

Added changelog file : https://github.com/manas-init/ansible/blob/patch-1/changelogs/fragments/71977-ansible-galaxy-role-init.yml

greg-hellings

This is a priority bug preventing downstream testing from working properly in many places.

sivel · 2020-10-27T16:53:42Z

Could you please guide me on how to do the above mentioned.

Typically speaking, you would use an integration test. We already have a directory at test/integration/targets/ansible-galaxy

You would effectively add a test that roughly replicates the issue originally reported. It should fail without the fix, and pass with the fix.

Akasurde · 2020-12-21T16:00:02Z

@manas-init Are you still working on this? Thanks.

ansible-galaxy role init --role-skeleton=ansible-role-skeleton example fails. The issue was because 'dir' variable was changed to reference to a different list, but since it is a global variable so the update was causing issues. Now updating 'dir' variable to change value of variable in local scope. Fixes: ansible#71977

Akasurde · 2021-01-21T14:19:22Z

@bcoca I updated the test, could you please take a look?

webknjaz · 2021-01-21T16:12:55Z

lib/ansible/cli/galaxy.py

@@ -912,7 +912,7 @@ def execute_init(self):
            else:
                in_templates_dir = rel_root_dir == 'templates'

-            dirs = [d for d in dirs if not any(r.match(d) for r in skeleton_ignore_re)]
+            dirs[:] = [d for d in dirs if not any(r.match(d) for r in skeleton_ignore_re)]


Could anybody explain why this works/necessary? I think it deserves a code comment. It looks to me like it exploits some side-effect of changing the list in-place instead of reassigning the object but I don't get why this fixes the bug.

I believe it is due to mutating the dirs created by os.walk, which impacts future iterations that include that same dirs reference.

I'm sure we could refactor the code, to not require it, but this is a known good change that works.

After looking at the PR description, it sounds like it'd be saner to use a different variable name. Also, it looks like this could be moved closer to the loop that iterates over these dirs.

I think that's the point, it needs to mutate the existing reference, or the code needs refactored in a larger scale.

pbpblush · 2021-02-02T18:08:11Z

Hi there, I found that this may also be fixed by updating ansible/config/base.yml

changing line 1414 to be:

default: ["^.git$", "^.git/.*$", "^.*/.git_keep$"]

this would be a smaller change

s-hertel · 2021-02-24T21:48:02Z

@pbpblush This is a one-liner too 😄 Just has tests and changelog and a little baggage. Using the configuration setting is a good workaround, but I think we should fix it more generally like this. The setting matches files and directory names, so if "^.git$" matches a directory, the files inside of it should also be ignored. It doesn't look like the docs mention that the setting applies to directory names, but it looks intentional: c20beee.

Anyone ignoring a directory name could experience this bug. For example, ANSIBLE_GALAXY_ROLE_SKELETON_IGNORE='^ignored_dir$' would skip the directory still but try to add any files inside of it, which causes the traceback, whereas before os.walk wouldn't consider the contents of skipped directories at all because we mutated the list instead of reassigning it.

Addressed

* galaxy: restore left hand slicing in assignment Fix 'ansible-galaxy role init --role-skeleton=role-skeleton' when the role skeleton contains an ignored directory. The issue was because the 'dirs' variable was changed to reference a different list, but needs to be mutated instead to stop os.walk from traversing ignored directories. Fixes: ansible#71977 Co-authored-by: Abhijeet Kasurde <akasurde@redhat.com> (cherry picked from commit eb72c36)

) * galaxy: restore left hand slicing in assignment Fix 'ansible-galaxy role init --role-skeleton=role-skeleton' when the role skeleton contains an ignored directory. The issue was because the 'dirs' variable was changed to reference a different list, but needs to be mutated instead to stop os.walk from traversing ignored directories. Fixes: #71977 Co-authored-by: Abhijeet Kasurde <akasurde@redhat.com> (cherry picked from commit eb72c36) Co-authored-by: manas-init <70483021+manas-init@users.noreply.github.com>

manas-init mentioned this pull request Sep 30, 2020

Error "ansible-galaxy role init" #71977

Closed

bcoca removed the needs_triage Needs a first human triage before being processed. label Oct 1, 2020

bcoca previously requested changes Oct 1, 2020

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Oct 1, 2020

bcoca mentioned this pull request Oct 1, 2020

Fix: [Errno 2] No such file or directory #72046

Closed

manas-init requested a review from bcoca October 3, 2020 06:38

ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Oct 11, 2020

greg-hellings approved these changes Oct 27, 2020

View reviewed changes

Akasurde force-pushed the patch-1 branch from 3691f9c to 2dd565e Compare January 21, 2021 13:06

Akasurde changed the title ~~Update galaxy.py~~ galaxy: Handle hidden git directories in role skeleton Jan 21, 2021

webknjaz reviewed Jan 21, 2021

View reviewed changes

ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Jan 29, 2021

Add a comment about why [:] is necessary

03f5523

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 24, 2021

s-hertel merged commit eb72c36 into ansible:devel Feb 24, 2021

ansible locked and limited conversation to collaborators Mar 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

galaxy: Handle hidden git directories in role skeleton #72035

galaxy: Handle hidden git directories in role skeleton #72035

manas-init commented Sep 30, 2020 •

edited by bcoca

Loading

bcoca left a comment

manas-init commented Oct 1, 2020

manas-init commented Oct 1, 2020

greg-hellings left a comment

sivel commented Oct 27, 2020

Akasurde commented Dec 21, 2020

Akasurde commented Jan 21, 2021

webknjaz Jan 21, 2021

sivel Jan 21, 2021

webknjaz Jan 21, 2021

sivel Jan 21, 2021

pbpblush commented Feb 2, 2021

s-hertel commented Feb 24, 2021

galaxy: Handle hidden git directories in role skeleton #72035

galaxy: Handle hidden git directories in role skeleton #72035

Conversation

manas-init commented Sep 30, 2020 • edited by bcoca Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

bcoca left a comment

Choose a reason for hiding this comment

manas-init commented Oct 1, 2020

manas-init commented Oct 1, 2020

greg-hellings left a comment

Choose a reason for hiding this comment

sivel commented Oct 27, 2020

Akasurde commented Dec 21, 2020

Akasurde commented Jan 21, 2021

webknjaz Jan 21, 2021

Choose a reason for hiding this comment

sivel Jan 21, 2021

Choose a reason for hiding this comment

webknjaz Jan 21, 2021

Choose a reason for hiding this comment

sivel Jan 21, 2021

Choose a reason for hiding this comment

pbpblush commented Feb 2, 2021

s-hertel commented Feb 24, 2021

manas-init commented Sep 30, 2020 •

edited by bcoca

Loading