From ff6e4da36addccb06001f7b05b1a9c04ae1d7984 Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Tue, 4 Oct 2022 11:47:32 -0400 Subject: [PATCH] fixes to FA inheritance (#78990) finalized applies to all field attributes fix getting parent value also remove unused/needed extend/prepend signature moar testing --- lib/ansible/playbook/base.py | 19 ++++------ lib/ansible/playbook/block.py | 10 ++++-- lib/ansible/playbook/task.py | 10 ++++-- test/integration/targets/omit/C75692.yml | 44 ++++++++++++++++++++++++ test/integration/targets/omit/runme.sh | 6 ++++ 5 files changed, 72 insertions(+), 17 deletions(-) create mode 100644 test/integration/targets/omit/C75692.yml diff --git a/lib/ansible/playbook/base.py b/lib/ansible/playbook/base.py index bb7a29a0da9..9daccbbd714 100644 --- a/lib/ansible/playbook/base.py +++ b/lib/ansible/playbook/base.py @@ -479,24 +479,18 @@ class FieldAttributeBase: return value def set_to_context(self, name): + ''' set to parent inherited value or Sentinel as appropriate''' attribute = self.fattributes[name] if isinstance(attribute, NonInheritableFieldAttribute): - self.set_to_default(name) + # setting to sentinel will trigger 'default/default()' on getter + setattr(self, name, Sentinel) else: try: - setattr(self, name, self._get_parent_attribute(name)) + setattr(self, name, self._get_parent_attribute(name, omit=True)) except AttributeError: - # mostly playcontext as only tasks/handlers really resolve to parent - self.set_to_default(name) - - def set_to_default(self, name): - - attribute = self.fattributes[name] - if callable(attribute.default): - setattr(self, name, attribute.default()) - else: - setattr(self, name, attribute.default) + # mostly playcontext as only tasks/handlers/blocks really resolve parent + setattr(self, name, Sentinel) def post_validate(self, templar): ''' @@ -545,7 +539,6 @@ class FieldAttributeBase: # or default specified in the FieldAttribute and move on if omit_value is not None and value == omit_value: self.set_to_context(name) - self._finalized = True continue # and make sure the attribute is of the type it should be diff --git a/lib/ansible/playbook/block.py b/lib/ansible/playbook/block.py index 7a6080cd466..216d04e856e 100644 --- a/lib/ansible/playbook/block.py +++ b/lib/ansible/playbook/block.py @@ -294,14 +294,20 @@ class Block(Base, Conditional, CollectionSearch, Taggable): for dep in dep_chain: dep.set_loader(loader) - def _get_parent_attribute(self, attr, extend=False, prepend=False): + def _get_parent_attribute(self, attr, omit=False): ''' Generic logic to get the attribute or parent attribute for a block value. ''' extend = self.fattributes.get(attr).extend prepend = self.fattributes.get(attr).prepend + try: - value = getattr(self, f'_{attr}', Sentinel) + # omit self, and only get parent values + if omit: + value = Sentinel + else: + value = getattr(self, f'_{attr}', Sentinel) + # If parent is static, we can grab attrs from the parent # otherwise, defer to the grandparent if getattr(self._parent, 'statically_loaded', True): diff --git a/lib/ansible/playbook/task.py b/lib/ansible/playbook/task.py index 47072b069fd..d0a4c6ecc8d 100644 --- a/lib/ansible/playbook/task.py +++ b/lib/ansible/playbook/task.py @@ -457,14 +457,20 @@ class Task(Base, Conditional, Taggable, CollectionSearch): if self._parent: self._parent.set_loader(loader) - def _get_parent_attribute(self, attr, extend=False, prepend=False): + def _get_parent_attribute(self, attr, omit=False): ''' Generic logic to get the attribute or parent attribute for a task value. ''' extend = self.fattributes.get(attr).extend prepend = self.fattributes.get(attr).prepend + try: - value = getattr(self, f'_{attr}', Sentinel) + # omit self, and only get parent values + if omit: + value = Sentinel + else: + value = getattr(self, f'_{attr}', Sentinel) + # If parent is static, we can grab attrs from the parent # otherwise, defer to the grandparent if getattr(self._parent, 'statically_loaded', True): diff --git a/test/integration/targets/omit/C75692.yml b/test/integration/targets/omit/C75692.yml new file mode 100644 index 00000000000..6e1215fbb5b --- /dev/null +++ b/test/integration/targets/omit/C75692.yml @@ -0,0 +1,44 @@ +- hosts: localhost + gather_facts: false + tasks: + - name: Make sure foo is gone + file: + path: foo + state: absent + - name: Create foo - should only be changed in first iteration + copy: + dest: foo + content: foo + check_mode: '{{ omit }}' + register: cmode + loop: + - 1 + - 2 + + - when: ansible_check_mode + block: + - name: stat foo + stat: path=foo + register: foo + check_mode: off + - debug: var=foo + - name: validate expected outcomes when in check mode and file does not exist + assert: + that: + - cmode['results'][0] is changed + - cmode['results'][1] is changed + when: not foo['stat']['exists'] + + - name: validate expected outcomes when in check mode and file exists + assert: + that: + - cmode['results'][0] is not changed + - cmode['results'][1] is not changed + when: foo['stat']['exists'] + + - name: validate expected outcomes when not in check mode (file is always deleted) + assert: + that: + - cmode['results'][0] is changed + - cmode['results'][1] is not changed + when: not ansible_check_mode diff --git a/test/integration/targets/omit/runme.sh b/test/integration/targets/omit/runme.sh index a335dd8dfeb..e2f3c0274d9 100755 --- a/test/integration/targets/omit/runme.sh +++ b/test/integration/targets/omit/runme.sh @@ -2,4 +2,10 @@ set -eux +# positive inheritance works ANSIBLE_ROLES_PATH=../ ansible-playbook 48673.yml 75692.yml -i ../../inventory -v "$@" + +# ensure negative also works +ansible-playbook -C C75692.yml -i ../../inventory -v "$@" # expects 'foo' not to exist +ansible-playbook C75692.yml -i ../../inventory -v "$@" # creates 'foo' +ansible-playbook -C C75692.yml -i ../../inventory -v "$@" # expects 'foo' does exist