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 enclosing container p2p-primary-paths name #1235

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

Conversation

Pull-eckermann
Copy link

Fix enclosing container p2p-primary-paths name.

Enclosing container p2p-primary-path name is not in the format described in https://www.openconfig.net/docs/guides/style_guide/#yang-style-conventions.

@Pull-eckermann Pull-eckermann requested a review from a team as a code owner December 19, 2024 20:17
@Pull-eckermann
Copy link
Author

Pull-eckermann commented Jan 15, 2025

Please, can someone review it?

@romeyod
Copy link
Contributor

romeyod commented Jan 16, 2025

This seems like a breaking change to me. I understand the motivation of being yang style complaint but seems to me we have missed that opportunity with this one.
@dplore can correct me here.

@jsterne
Copy link

jsterne commented Jan 16, 2025

This seems like a breaking change to me. I understand the motivation of being yang style complaint but seems to me we have missed that opportunity with this one. @dplore can correct me here.

I'd tend to agree. Non backwards compatible changes shouldn't be made lightly. They impact everyone who has automated/integrated around this area. I'm doubtful this one is ever worth it. But I'd imagine a variety of opinions exist around something like this :-)

@earies
Copy link
Contributor

earies commented Jan 16, 2025

This seems like a breaking change to me. I understand the motivation of being yang style complaint but seems to me we have missed that opportunity with this one. @dplore can correct me here.

I'd tend to agree. Non backwards compatible changes shouldn't be made lightly. They impact everyone who has automated/integrated around this area. I'm doubtful this one is ever worth it. But I'd imagine a variety of opinions exist around something like this :-)

+1 - this is definitely a breaking change and quick glance looks like this subtree/list has already been shipping across various implementations for a while now. Yes it slipped through the cracks but if we weigh the pros and cons here to going back and fixing this now, you would likely find a strong preference towards preserving compatibility vs. style conformance.

This is especially true for a good chunk of the industry that ingests OpenConfig modeled data unknowingly. All that is seen is a breaking change to a request or an alternate dataset now being created as a result.

It's not to say we should be held to never iterating in such fashion but there is an elephant in the room here that needs proper attention on proper agreed upon operational transition methods and signals of such changes.

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

Successfully merging this pull request may close these issues.

4 participants