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

Add module for mgmt:OOB to contract binding (mgmtRsOoBProv) (DCNE-212) #696

Closed
wants to merge 6 commits into from

Conversation

edudppaz
Copy link
Contributor

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

Adds a new module for binding of out-of-band EPG to a provided contract. As the aci_epg_to_contract does not apply to OOB epgs

New or Affected Module(s):

  • aci_oob_epg_to_contract

APIC version and APIC Platform

Tested on:

  • V 5.2(8e) and on-prem
  • All the cloud APICs in the network-integration tests

Collection versions

  • cisco.aci x.x.x

References

  • #0000

@akinross akinross added the jira-sync Sync this issue to Jira label Oct 17, 2024
@github-actions github-actions bot changed the title Add module for mgmt:OOB to contract binding (mgmtRsOoBProv) Add module for mgmt:OOB to contract binding (mgmtRsOoBProv) (DCNE-212) Oct 17, 2024
@akinross
Copy link
Collaborator

black formatting failed, please format code with black

@edudppaz
Copy link
Contributor Author

This should fix #654 and superseeds #658

@akinross
Copy link
Collaborator

This should fix #654 and superseeds #658

I have contacted the raiser of this PR and tomorrow will have a meeting to see his progress on the topic. I will keep you posted on the results of this.

@akinross
Copy link
Collaborator

Hi @edudppaz,

I think we should make a decision here before we proceed with review.

Currently in the collection we have the module aci_node_mgmt_epg to configure in-band and out-of-band mgmt EPGs. I think there are 2 ways we can proceed to expose the contract functionality.

  1. expose a combined aci_node_mgmt_epg_to_contract that exposes both in-band and out-of-band mgmt EPGs with their corresponding contracts ( bit more complexity mainly from the inband side but this is abstracted away from the user )
  2. expose aci_oob_mgmt_epg_to_contract and aci_inb_mgmt_epg_to_contract module and see them as separate modules

My personal preference would be option 1 since this is then in line with the current module structure. What is your thought on this?

I had a meeting with the PR raiser of #658. Adjustments can be made from that side to conform with option 2. He is in parallel also working on a aci_oob_contract module (nearing completion, just requires documentation updates).

@shrsr, @samiib, @sajagana, @anvitha-jain, @gmicol, @lhercot what are your your thoughts?

@edudppaz
Copy link
Contributor Author

Hi @edudppaz,

I think we should make a decision here before we proceed with review.

Currently in the collection we have the module aci_node_mgmt_epg to configure in-band and out-of-band mgmt EPGs. I think there are 2 ways we can proceed to expose the contract functionality.

1. expose a combined aci_node_mgmt_epg_to_contract that exposes both in-band and out-of-band mgmt EPGs with their corresponding contracts ( bit more complexity mainly from the inband side but this is abstracted away from the user )

2. expose aci_oob_mgmt_epg_to_contract and aci_inb_mgmt_epg_to_contract module and see them as separate modules

My personal preference would be option 1 since this is then in line with the current module structure. What is your thought on this?

I had a meeting with the PR raiser of #658. Adjustments can be made from that side to conform with option 2. He is in parallel also working on a aci_oob_contract module (nearing completion, just requires documentation updates).

@shrsr, @samiib, @sajagana, @anvitha-jain, @gmicol, @lhercot what are your your thoughts?

Hi :) both options work for me , so its ok. I did this one as i saw the other PR have not been changed since 5 months ago, and the code was focused on the in-band side but nothing with oob, as it is using variables as contract_types, priority, etc that are not needed for the epg to contract binding, only for the contract itself.

The in-band EPG is more a "normal" epg (with a bridge-domain, provided/consumed) etc than the out-of-band, so if the contract binding will be kept in a single module , as you say, we need to split the whole logic with an "if" matching the epg type. Maybe we can just join both PRs in a single one and reuse the oob code here and the in-band code from Ziaf

@samiib
Copy link
Collaborator

samiib commented Oct 21, 2024

@akinross and @edudppaz both options are okay. However, I do prefer option 1.
Having both in-band and out-of-band mgmt EPGs in one module is consistent with the current module structure.

@sajagana
Copy link
Collaborator

Hi @akinross, Option 1 will be good.

@Ziaf007
Copy link

Ziaf007 commented Oct 21, 2024

I can work on getting this done. Already have modules ready for inband and OOB, need to join them in one module now.

@edudppaz
Copy link
Contributor Author

Great @Ziaf007 then this PR can be closed :)

@shrsr
Copy link
Collaborator

shrsr commented Oct 21, 2024

@akinross I would go with option 1.

@gmicol
Copy link
Collaborator

gmicol commented Oct 21, 2024

I have a preference for option 1 @akinross. It's more consistent with the way we build modules.

@akinross
Copy link
Collaborator

Option 1 is chosen, closing this PR. Follow #658 for progress.

@akinross akinross closed this Oct 22, 2024
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.

7 participants