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

Fix reading resource params with nested spec #455

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

Conversation

ggverma
Copy link
Contributor

@ggverma ggverma commented Feb 28, 2023

In Policy modules, the resources are nested. Because of this, we read the resource params from the supplied Ansible Playbook and manually validate it with the resource spec. However, we were not reading the resource params correctly if they were nested. This patch fixes that issue. In addition, we convert the new resource params and existing resource params into Ordered Data so that we can compare them and infer if there are any updates with better accuracy

Issue: #417

@vmwclabot
Copy link
Member

@ggverma, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

if params:
filtered_params[key] = params
elif current_spec_type == 'list' and v:
if isinstance(v[0], dict):
Copy link
Member

Choose a reason for hiding this comment

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

This might deserve a comment for improving readability.

for instance - assuming that if the first element of the list is a dict, then all the other element will be dicts as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, they will be dict though because spec looks like this in such case:

                next_hops=dict(
                    required=True,
                    type='list', <--------- current_spec_type
                    elements='dict', <--- elements can be all str, dict, or list. This defaults to str
                    options=dict(
                        admin_distance=dict(
                            type='int',
                            default=1
                        ),
                        ip_address=dict(
                            type='str'
                        ),
                        scope=dict(
                            type='list',
                            elements='str'
                        )
                    )
                ),

I have updated the condition so that the comment is not needed.

@vmwclabot
Copy link
Member

@ggverma, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

@vmwclabot
Copy link
Member

@ggverma, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

In Policy modules, the resources are nested. Because of this, we read
the resource params from the supplied Ansible Playbook and manually
validate it with the resource spec. However, we were not reading the
resource params correctly if they were nested. This patch fixes that
issue. In addition, we convert the new resource params and existing
resource params into Ordered Data so that we can compare them and infer
if there are any updates with better accuracy

Issue: vmware#417

Signed-off-by: Gautam Verma <vermag@vmware.com>
Signed-off-by: Gautam Verma <gautam94verma@gmail.com>
nebb00 added a commit to nebb00/ansible-for-nsxt that referenced this pull request Sep 2, 2024
Signed-off-by: Ben Rosenqvist <nebb@protonmail.com>
@brosenqvist
Copy link

brosenqvist commented Oct 28, 2024

this fixes a problem I had with bridge_profiles in segments not being idempotent, but creates issues with tier0, in which is throws KeyError: 'edge_node_info', been trying to resolve, but failing. Any help would be great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants