Remove sonic ports defaults #323
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robertvolkmann
requested changes
Sep 26, 2024
Comment on lines
111
to
119
| {% set port = sonic_ports_dict[name] %} | ||
| {% if (port is defined and port.speed is defined) or running_cfg.speed is defined %} | ||
| speed: "{{ (port | default({})).speed | default(running_cfg.speed) }}" | ||
| {% endif %} | ||
| {% if (port is defined and port.mtu is defined) or running_cfg.mtu is defined %} | ||
| mtu: "{{ (port | default({})).mtu | default(running_cfg.mtu) }}" | ||
| {% endif %} | ||
| {% if (port is defined and port.fec is defined) or running_cfg.fec is defined %} | ||
| fec: "{{ (port | default({})).fec | default(running_cfg.fec) }}" |
There was a problem hiding this comment.
Could you try?
Suggested change
| {% set port = sonic_ports_dict[name] %} | |
| {% if (port is defined and port.speed is defined) or running_cfg.speed is defined %} | |
| speed: "{{ (port | default({})).speed | default(running_cfg.speed) }}" | |
| {% endif %} | |
| {% if (port is defined and port.mtu is defined) or running_cfg.mtu is defined %} | |
| mtu: "{{ (port | default({})).mtu | default(running_cfg.mtu) }}" | |
| {% endif %} | |
| {% if (port is defined and port.fec is defined) or running_cfg.fec is defined %} | |
| fec: "{{ (port | default({})).fec | default(running_cfg.fec) }}" | |
| {% set port = sonic_ports_dict.get(name, {}) %} | |
| {% if port.speed is defined or running_cfg.speed is defined %} | |
| speed: "{{ port.speed | default(running_cfg.speed) }}" | |
| {% endif %} | |
| {% if port.mtu is defined or running_cfg.mtu is defined %} | |
| mtu: "{{ port.mtu | default(running_cfg.mtu) }}" | |
| {% endif %} | |
| {% if port.fec is defined or running_cfg.fec is defined %} | |
| fec: "{{ port.fec | default(running_cfg.fec) }}" |
| @@ -107,14 +107,16 @@ PORT: | |||
| {% if 'parent_port' in running_cfg %} | |||
| parent_port: {{ running_cfg.parent_port }} | |||
| {% endif %} | |||
| {% if sonic_ports_dict[name] is defined %} | |||
| {% set port = sonic_ports_dict[name] %} | |||
| admin_status: up | |||
There was a problem hiding this comment.
Why always setting admin_status to up? Why not similar to fec, mtu, and speed?
There was a problem hiding this comment.
We could introduce another variable for the admin_status, but that would possibly involve a lot of changes to the existing deployments, right?
This reverts commit aa3080b.
|
Closing in favor of #324. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Additional Description
Currently, if the
sonic_portsvariable is defined the sonic role will require default values for fec, mtu and speed to be set. A more reasonable behavior, in my opinion, is to leave all settings of the running configuration in place, if no value was provided.Breaking Change