Skip to content

Addition of service graph modules and their test files (DCNE-410) #754

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

shrsr
Copy link
Collaborator

@shrsr shrsr commented May 6, 2025

No description provided.

@shrsr shrsr self-assigned this May 6, 2025
@shrsr shrsr added the jira-sync Sync this issue to Jira label May 6, 2025
@github-actions github-actions bot changed the title Addition of service graph modules and their test files Addition of service graph modules and their test files (DCNE-410) May 6, 2025
@shrsr shrsr requested a review from akinross May 8, 2025 20:36
- cisco.aci.annotation

notes:
- The I(tenant), I(graph), I(contract) and I(node) must exist before using this module in your playbook.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- The I(tenant), I(graph), I(contract) and I(node) must exist before using this module in your playbook.
- The O(tenant), O(graph), O(contract) and O(node) must exist before using this module in your playbook.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comment was already made by @gmicol in a related PR. In ansible ACI collection we've been using I() for Input in the other modules. I believe that this usage should be consistent within this collection.

type: str
permit_handoff:
description:
- Indicates whether to allow handoff of traffic to the associated logical interface.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does APIC default these Booleans to anything when unset? permit_handoff, acl, rule_type?

Comment on lines +387 to +396
# child_configs.append(
# {
# "vnsRsLIfCtxToBD": {
# "attributes": {
# "dn": child.get("vnsRsLIfCtxToBD").get("attributes").get("dn"),
# "status": "deleted",
# }
# }
# }
# )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# child_configs.append(
# {
# "vnsRsLIfCtxToBD": {
# "attributes": {
# "dn": child.get("vnsRsLIfCtxToBD").get("attributes").get("dn"),
# "status": "deleted",
# }
# }
# }
# )

Not sure if you need the previous code. I think the comment is fine.

Copy link
Collaborator Author

@shrsr shrsr May 11, 2025

Choose a reason for hiding this comment

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

We left the code commented in other modules as well. I left it to be consistent with them..

- cisco.aci.aci
- cisco.aci.annotation
notes:
- The I(tenant), I(contract), I(graph), I(device) and I(node) must exist before using this module in your playbook.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- The I(tenant), I(contract), I(graph), I(device) and I(node) must exist before using this module in your playbook.
- The O(tenant), O(contract), O(graph), O(device) and O(node) must exist before using this module in your playbook.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same comment as above

- cisco.aci.annotation
- cisco.aci.owner
notes:
- The I(tenant) must exist before using this module in your playbook.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- The I(tenant) must exist before using this module in your playbook.
- The O(tenant) must exist before using this module in your playbook.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same comment as above

choices: [ redirect, unspecified ]
is_copy:
description:
- Whether the device is a copy device.
Copy link
Collaborator

Choose a reason for hiding this comment

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

default?

type: bool
share_encap:
description:
- Whether to share encapsulation across the service graph.
Copy link
Collaborator

Choose a reason for hiding this comment

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

default?

- cisco.aci.annotation
- cisco.aci.owner
notes:
- The I(tenant), I(service_graph) and I(device) must exist before using this module in your playbook.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- The I(tenant), I(service_graph) and I(device) must exist before using this module in your playbook.
- The O(tenant), O(service_graph) and O(device) must exist before using this module in your playbook.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same comment as above

- cisco.aci.annotation
- cisco.aci.owner
notes:
- The I(tenant) and I(service_graph) must exist before using this module in your playbook.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- The I(tenant) and I(service_graph) must exist before using this module in your playbook.
- The O(tenant) and O(service_graph) must exist before using this module in your playbook.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same comment as above

@shrsr shrsr requested a review from samiib May 11, 2025 23:44
Copy link
Collaborator

@akinross akinross left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira-sync Sync this issue to Jira
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants