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

[minor_changes] Adding new module for physical interface (DCNE-50) #560

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

Conversation

anvitha-jain
Copy link
Collaborator

(object: interfaceProfiles)
solves #392

it required ndo_interface_seeting module #540

@anvitha-jain anvitha-jain self-assigned this Oct 30, 2024
@anvitha-jain anvitha-jain added enhancement New feature or request jira-sync Sync this issue to Jira labels Oct 30, 2024
@github-actions github-actions bot changed the title [minor_changes] Adding new module for physical interface [minor_changes] Adding new module for physical interface (DCNE-50) Oct 30, 2024
@akinross
Copy link
Collaborator

sanity fails

Copy link
Collaborator

@gmicol gmicol left a comment

Choose a reason for hiding this comment

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

Sanity tests failed

plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
@anvitha-jain anvitha-jain requested review from samiib and gmicol November 7, 2024 17:59
@anvitha-jain
Copy link
Collaborator Author

The sanity fails because of a function which doesn't exist, since this function is being added in another PR #539

plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Show resolved Hide resolved
@anvitha-jain anvitha-jain force-pushed the physical_interface branch 2 times, most recently from 54cabb7 to a710854 Compare November 12, 2024 18:32
@anvitha-jain anvitha-jain requested a review from shrsr November 12, 2024 18:34
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
plugins/modules/ndo_physical_interface.py Show resolved Hide resolved
gmicol
gmicol previously approved these changes Nov 19, 2024
Copy link
Collaborator

@gmicol gmicol left a comment

Choose a reason for hiding this comment

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

LGTM

plugins/modules/ndo_physical_interface.py Outdated Show resolved Hide resolved
gmicol
gmicol previously approved these changes Nov 22, 2024
Copy link
Collaborator

@gmicol gmicol left a comment

Choose a reason for hiding this comment

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

LGTM


if mso.existing:
proposed_payload = copy.deepcopy(match.details)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we leverage the new function append_update_ops_data() here?

@@ -318,6 +318,19 @@ def write_file(module, url, dest, content, resp, tmpsrc=None):
os.remove(tmpsrc)


def format_interface_descriptions(interface_descriptions, node=None):
formated_interface_descriptions = [
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
formated_interface_descriptions = [
formatted_interface_descriptions = [

for interface_description in interface_descriptions
]

return formated_interface_descriptions
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
return formated_interface_descriptions
return formatted_interface_descriptions

Comment on lines 360 to 365
payload = {
"name": name,
"nodes": nodes,
"interfaces": interfaces,
"policyGroupType": physical_interface_type,
}
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
payload = {
"name": name,
"nodes": nodes,
"interfaces": interfaces,
"policyGroupType": physical_interface_type,
}
mso_values = dict(
name = name,
nodes = nodes,
interfaces = interfaces,
policyGroupType = physical_interface_type,
)

Our team planned to maintain the variable naming consistency for the new modules.

@akinross akinross requested review from sajagana and shrsr December 16, 2024 07:39

update_path = "{0}/{1}".format(path, match.index)

if interface_descriptions == [] and proposed_payload.get("interfaceDescriptions"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a documentation note to explain the update and clear processes of the "interface_descriptions".

- nm_delete_physical_interface_again.previous == nm_delete_physical_interface_again.current == {}
- nm_delete_physical_interface_again.current.uuid is not defined

- name: Query all tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need this task? Change the task name.

- name: Update Physical Interface of type 'breakout' by removing interface interface_descriptions
cisco.mso.ndo_physical_interface:
<<: *update_physical_interface_uuid
interface_descriptions: []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a task to update interface_descriptions values before completely clearing it?
Could you add an element with an existing value and remove it?

check_mode: true
register: cm_physical_interface

- name: Create Physical Interface of type 'physical'
Copy link
Collaborator

Choose a reason for hiding this comment

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

CI appears broken, there are dangling objects for this policy without a template. This makes the testing fail.

Comment on lines +30 to +31
- name: Execute tasks only for MSO version > 4.3
when: version.current.version is version('4.3', '>=')
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be 4.4 I think, because now CI does not work

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should add a 4.4 instance to CI for testing this func

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

Successfully merging this pull request may close these issues.

Enhancement: New module for Physical Interface in Fabric Resource Policies Template (DCNE-50)
6 participants