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

Sanitize debug var= tasks of the loop variable #66269

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

AlanCoding
Copy link
Member

@AlanCoding AlanCoding commented Jan 8, 2020

SUMMARY

Proposed fix for #65856

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

debug

ADDITIONAL INFORMATION

I have a test case for this, modeled after

[ "$(ansible-playbook label.yml "$@" |grep 'item='|sed -e 's/^.*(item=looped_var \(.*\)).*$/\1/')" == "${MATCH}" ]

./runme.sh

(you have to manually inspect the exit code in that case)

inside runme.sh

[ "$(ansible-playbook -i localhost, loop_test.yml | grep 'value2')" == "" ]

inside loop_test.yml

- name: loop_control/debug
  hosts: localhost
  gather_facts: false
  connection: local
  vars:
    my_var: foo
    loop_me:
      - attr1: value1
        attr2: value2
  tasks:

    - name: Echo a var inside of a loop, should only print the label value1
      debug: var=my_var
      loop: "{{ loop_me }}"
      loop_control:
        label: "{{ foo_bar.attr1 }}"
        loop_var: "foo_bar"

    - name: Echo text inside of a loop, should not print anything in loop_me
      debug: msg='hello'
      loop: "{{ loop_me }}"
      loop_control:
        label: "{{ foo_bar.attr1 }}"
        loop_var: "foo_bar"

I am less confident about integrating this into the tests, although there really shouldn't be a problem with it.

and I confirm exit code 1 with devel and exit code 0 with this patch.

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jan 8, 2020
@@ -263,6 +263,10 @@ def _clean_results(self, result, task_name):
# 'var' value as field, so eliminate others and what is left should be varname
for hidme in self._hide_in_debug:
result.pop(hidme, None)
# when using loop control, remove whatever is under loop_var
if 'ansible_loop_var' in result:
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to do the same for ansible_index_var variable too.

Copy link
Member Author

Choose a reason for hiding this comment

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

good call, added that to the test playbook and found it show up. Pushing change now.

Copy link
Member

@bcoca bcoca May 13, 2020

Choose a reason for hiding this comment

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

actually, any var that is not the 'item' in vars= should be removed

@AlanCoding
Copy link
Member Author

I found that this afflicts Ansible 2.9 as well, since it's a bug fix, I'm going to add the changelog

@ansibot ansibot added support:community This issue/PR relates to code supported by the Ansible community. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed small_patch core_review In order to be merged, this PR must follow the core review workflow. labels Jan 8, 2020
@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jan 9, 2020
@bcoca bcoca added the P3 Priority 3 - Approved, No Time Limitation label Jan 14, 2020
@bcoca bcoca self-requested a review January 14, 2020 15:26
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Jan 14, 2020
@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 Jan 22, 2020
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels May 6, 2020
@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label May 13, 2020
@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 May 21, 2020
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Sep 4, 2020
@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 Apr 29, 2021
@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 ci_verified Changes made in this PR are causing tests to fail. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jun 26, 2023
@mattclay
Copy link
Member

/azp run

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 66269 in repo ansible/ansible

@mattclay
Copy link
Member

This PR needs to be rebased before CI can run.

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jul 13, 2023
@webknjaz
Copy link
Member

@mattclay that ci_verified label doesn't exclude PRs from the “stuck” queue, FYI.

@mattclay
Copy link
Member

/azp run

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 66269 in repo ansible/ansible

@mattclay
Copy link
Member

After seeing the No commit pushedDate error on another PR, where one /azp run comment failed and a second one passed, I figured I'd try again. Looking at the GH API output for https://api.github.com/repos/ansible/ansible/pulls/66269 I see pushed_at dates -- I don't know if that's the same as what AZP is looking for when it's looking for pushedDate.

@mattclay
Copy link
Member

I'll try closing and re-opening to see if that changes things.

@mattclay mattclay closed this Jul 14, 2023
@mattclay mattclay reopened this Jul 14, 2023
@mattclay
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mattclay
Copy link
Member

That seems to have worked. So perhaps the No commit pushedDate error is due to something on the GH side of things.

@mattclay
Copy link
Member

@mattclay that ci_verified label doesn't exclude PRs from the “stuck” queue, FYI.

Yeah, I wanted to see if the new bot would remove the label or not. If not, I was thinking we might want to update our search filter to exclude ci_verified when searching for needs_ci -- but now it seems that may not be necessary. I'm going to try updating the other PRs with a close/re-open as well to see if they'll report results.

This PR still needs a rebase to have a chance at passing CI. That's no longer due to the No commit pushedDate error, but because it's old enough that the shallow clone we're doing isn't finding the merge base.

@ansibot ansibot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. ci_verified Changes made in this PR are causing tests to fail. labels Jul 14, 2023
@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jul 14, 2023
@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 Jul 25, 2023
@webknjaz
Copy link
Member

webknjaz commented Nov 4, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Nov 4, 2024
@webknjaz
Copy link
Member

webknjaz commented Nov 5, 2024

@AlanCoding could you rebase?

@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Nov 5, 2024
@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 Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. P3 Priority 3 - Approved, No Time Limitation stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants