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

Update description of the model-name leaf. #1160

Merged
merged 22 commits into from
Sep 13, 2024

Conversation

SydneyCaulfeild
Copy link
Contributor

Change Scope

  • Updating the description of the model-name leaf to help distinguish it from the part-no leaf.
  • This change is backwards compatible.

@SydneyCaulfeild SydneyCaulfeild requested a review from a team as a code owner August 8, 2024 14:53
@dplore
Copy link
Member

dplore commented Aug 26, 2024

/gcbrun

@OpenConfigBot
Copy link

OpenConfigBot commented Aug 26, 2024

No major YANG version changes in commit 11c36ff

@dplore
Copy link
Member

dplore commented Aug 28, 2024

/gcbrun

Copy link
Member

@dplore dplore left a comment

Choose a reason for hiding this comment

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

some wordsmithing suggestions

release/models/platform/openconfig-platform.yang Outdated Show resolved Hide resolved
release/models/platform/openconfig-platform.yang Outdated Show resolved Hide resolved
@dplore
Copy link
Member

dplore commented Aug 29, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Aug 30, 2024

model-name as a mandatory field when removable = true is the intent. I think this is good. It's a new precedent in OC as the few places we use mandatory are not conditional.

I agree that removable is the better criteria than component type FRU which is overloaded and in practice couldn't be used as a conditional in practice because un-orderable parts are often represented using FRU as Ebben has noted.

@dplore
Copy link
Member

dplore commented Aug 30, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Aug 30, 2024

model-name as a mandatory field when removable = true is the intent. I think this is good. It's a new precedent in OC as the few places we use mandatory are not conditional.

Well, looks like our tool-chain in general does not support the mandatory field inside a where clause. (See the CI checks). I think a description stating this is mandatory when the component is 'removable' will need to suffice.

@earies
Copy link
Contributor

earies commented Aug 30, 2024

model-name as a mandatory field when removable = true is the intent. I think this is good. It's a new precedent in OC as the few places we use mandatory are not conditional.

Well, looks like our tool-chain in general does not support the mandatory field inside a where clause. (See the CI checks). I think a description stating this is mandatory when the component is 'removable' will need to suffice.

This is failing because as-is currently is invalid YANG. A mandatory stmt cannot be a substatment of a when clause and must exist as a sibling. I believe when this is fixed, the toolchain should accept this as it is no different than other usage to date.

e.g.

    leaf model-name {
      when "../removable = 'true'";
      mandatory true;
      type string;
      description
        "Model name that would be found in a catalog of stock keeping
        units (SKU) and should be the orderable name of the
        component.";
    }

@SydneyCaulfeild
Copy link
Contributor Author

Ok I have updated the mandatory stmt to not be nested in the when - @earies just want to confirm though that this leaf is still be optional even when ../removable is false?

Also, does anyone know why in openconfig-vlan.yang I see an example of something nested in the when? Is it because descriptions are allowed in the when statement but not a mandatory?

@earies
Copy link
Contributor

earies commented Sep 3, 2024

Ok I have updated the mandatory stmt to not be nested in the when - @earies just want to confirm though that this leaf is still be optional even when ../removable is false?

If removable=false then the model-name leaf is not permitted at all given the current logic

The following is permitted

<components xmlns="http://openconfig.net/yang/platform">
  <component>
    <name>C1</name>
    <removable>false</removable>
  </component>
  <component>
    <name>C2</name>
    <removable>true</removable>
    <model-name>M1</model-name>
  </component>
</components>

But either of the following is not

<components xmlns="http://openconfig.net/yang/platform">
  <component>
    <name>C1</name>
    <removable>true</removable>
  </component>
  <component>
    <name>C2</name>
    <removable>false</removable>
    <model-name>M1</model-name>
  </component>
</components>

Example yanglint validation on the latter:

libyang err : When condition "../removable = 'true'" not satisfied. (/openconfig-platform:components/component[name='C2']/model-name)
libyang err : Mandatory node "model-name" instance does not exist. (/openconfig-platform:components/component[name='C1'])
  • If not removable then model-name is not permitted whatsoever, it is not optional
  • If removable, then model-name is mandatory

Also, does anyone know why in openconfig-vlan.yang I see an example of something nested in the when? Is it because descriptions are allowed in the when statement but not a mandatory?

See the grammar defined in RFC 6020. Only the following substatements are permitted and optional.

  when-stmt           = when-keyword sep string optsep
                           (";" /
                            "{" stmtsep
                                ;; these stmts can appear in any order
                                [description-stmt stmtsep]
                                [reference-stmt stmtsep]
                             "}")

@dplore
Copy link
Member

dplore commented Sep 9, 2024

/gcbrun

@SydneyCaulfeild
Copy link
Contributor Author

Since "If not removable then model-name is not permitted whatsoever, it is not optional", I have updated the when statement to include an or for when the component is a chassis.

This is because some vendors, for example in an Arista DCS-7808-CH device, set openconfig/components/component/Chassis/state/removable, false. This makes sense since from my understanding you can't remove just a chassis, you'd have to remove the entire device.

However, I need the model-name to be available for chassis components (the model-name of the chassis here would be something similar to "DCS-7808-CH") so that I can determine which model a device is from the chassis' telemetry.

@dplore
Copy link
Member

dplore commented Sep 11, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Sep 11, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Sep 12, 2024

/gcbrun

@dplore dplore closed this Sep 12, 2024
@dplore dplore reopened this Sep 12, 2024
@dplore
Copy link
Member

dplore commented Sep 12, 2024

/gcbrun

@dplore dplore added the last-call PR that is in final review before merging. label Sep 12, 2024
@dplore
Copy link
Member

dplore commented Sep 12, 2024

This was reviewed in the OC Community meeting on Sep 12, 2024 without objection. Setting last call to Sep 13, 2024.

@dplore
Copy link
Member

dplore commented Sep 13, 2024

/gcbrun

@dplore dplore merged commit 27eabe3 into openconfig:master Sep 13, 2024
14 checks passed
@dplore dplore mentioned this pull request Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
last-call PR that is in final review before merging. non-breaking
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants