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.

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

copy module diff output for multi files #81346

Open
wants to merge 5 commits into
base: devel
Choose a base branch
from

Conversation

simonLeary42
Copy link

The copy module will now build a list of diffs as it iterates recursively over a directory.

addresses the following issue:

#74947

#75727

@ansibot ansibot added the needs_triage Needs a first human triage before being processed. label Jul 25, 2023
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Jul 27, 2023
Copy link
Contributor

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

Are you able to create a changelog fragment for this new feature https://docs.ansible.com/ansible/latest/community/development_process.html#creating-changelog-fragments.

It would also be great if we can add an integration test for this to just make sure it doesn't blow up with some untested branches. You can just add diff: true to the task level directives for an existing task that is copying multiple files.

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 27, 2023
@ansibot ansibot added the stale_review Updates were made after the last review and the last review is more than 7 days old. label Jul 27, 2023
@simonLeary42
Copy link
Author

simonLeary42 commented Jul 27, 2023

@jborean93
Copy link
Contributor

edit: that's not a recursive copy, disregard

Even so just adding diff: true to the task level directives (same indentation as name, module_name, register, etc) will ensure the diff code is actually executed and the final results are used as intended.

@simonLeary42
Copy link
Author

oh, I've mixed up check mode with diff mode.

@simonLeary42
Copy link
Author

simonLeary42 commented Jul 27, 2023

I'm not clear on how adding diff: true will ensure that the final results are used as intended. Shouldn't the correct output be stored somewhere so that the test output can be compared against it?

edit: I mixed check/diff up again...

@jborean93
Copy link
Contributor

I'm not clear on how adding check: true will ensure that the final results are used as intended

diff: true, it ensures the branches that creates the diff output is actually run in the test. While technically we probably should be asserting the output that's a bit ask so I'll settle for having the code running without breaking somewhere else. Say Ansible doesn't like the diff value as a list and so on.

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jul 27, 2023
@ansibot
Copy link
Contributor

ansibot commented Jul 27, 2023

The test ansible-test sanity --test changelog [explain] failed with 1 error:

changelogs/fragments/81346-copy-recursive-diff-fix:0:0: extension must be one of: .yml, .yaml

click here for bot help

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 27, 2023
@simonLeary42
Copy link
Author

I see a check failed like this:

04:01 --> Run command: ssh ansible-test@localhost 'rm -rf ~/.local/share/containers; STORAGE_DRIVER=overlay podman pull quay.io/bedrock/alpine:3.16.2'
04:01 time="2023-07-27T20:14:47Z" level=warning msg="\"/\" is not a shared mount, this could cause issues or missing mounts with rootless containers"
50:00 
50:00 NOTICE: Killed command to avoid an orphaned child process during handling of an unexpected exception.
50:00 Run command with stdout: ssh -i /home/AzDevOps_azpcontainer/.ansible/test/id_rsa -o BatchMode=yes -o ServerAliveCountMax=4 -o ServerAliveInterval=15 -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -q -vvv -E /tmp/ansible-test-ssh-debug-b8mlqqym.log -p 22 alpine@ec2-34-227-86-18.compute-1.amazonaws.com 'doas -n sh -c '"'"'sh -c '"'"'"'"'"'"'"'"'tar cf - -C /home/alpine/ansible/test --exclude .tmp results | gzip'"'"'"'"'"'"'"'"''"'"''
50:01 Run command with stdin: tar oxzf - -C /__w/3/ansible/test
50:01 FATAL: Tests aborted after exceeding the 50 minute time limit.
50:06 Stopped running alpine 3.17 (x86_64) [controller] @aws instance.
##[error]Bash exited with code '1'.
Finishing: Run Tests

I don't think that podman pull taking too long is a result of my diff: true

@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Jul 28, 2023
@simonLeary42
Copy link
Author

@jborean93 Can the CI be run again?

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 7, 2023
@simonLeary42
Copy link
Author

@jborean93 I wanted to give this a nudge in case it fell through the cracks. I'd appreciate your feedback when you get the chance

@simonLeary42
Copy link
Author

simonLeary42 commented Apr 14, 2024

to sidestep this broken contribution process I made my own collection: https://github.com/simonLeary42/ansible_copy_multi_diff

I couldn't figure out how to publish on ansible galaxy, there was no "my content" page or an "add an API key" function that I could find.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants