openconfig / ygot

A YANG-centric Go toolkit - Go/Protobuf Code Generation; Validation; Marshaling/Unmarshaling
Apache License 2.0
284 stars 107 forks source link

Ygot generates invalid enum values #286

Open mailravi02 opened 5 years ago

mailravi02 commented 5 years ago

I used ygot to convert openconfig-interfaces.yang to proto file. The value of the enum Operstatus was not properly generated by ygot. The same enum issue was observed in few other modules like lacp etc

Generated enum by ygot:

  enum OperStatus {
    OPERSTATUS_UNSET = 0;
    OPERSTATUS_UP = 2 [(yext.yang_name) = "UP"];
    OPERSTATUS_DOWN = 3 [(yext.yang_name) = "DOWN"];
    OPERSTATUS_TESTING = 4 [(yext.yang_name) = "TESTING"];
    OPERSTATUS_UNKNOWN = 5 [(yext.yang_name) = "UNKNOWN"];
    OPERSTATUS_DORMANT = 6 [(yext.yang_name) = "DORMANT"];
    OPERSTATUS_NOT_PRESENT = 7 [(yext.yang_name) = "NOT_PRESENT"];
    OPERSTATUS_LOWER_LAYER_DOWN = 8 [(yext.yang_name) = "LOWER_LAYER_DOWN"];
  }
   For example  OPERSTATUS_UP value is 1 and not 2 in openconfig-interfaces.yang.

Enum in yang:

leaf oper-status {
  type enumeration {
    enum UP {
      value 1;
      description
        "Ready to pass packets.";
    }
    enum DOWN {
      value 2;
      description
        "The interface does not pass any packets.";
    }
    enum TESTING {
      value 3;
      description
        "In some test mode.  No operational packets can
         be passed.";
    }
    enum UNKNOWN {
      value 4;
      description
        "Status cannot be determined for some reason.";
    }
    enum DORMANT {
      value 5;
      description
        "Waiting for some external event.";
    }
    enum NOT_PRESENT {
      value 6;
      description
        "Some component (typically hardware) is missing.";
    }
    enum LOWER_LAYER_DOWN {
      value 7;
      description
        "Down due to state of lower-layer interface(s).";
    }
  }
mailravi02 commented 5 years ago

I have attached other invalid enum values generated by ygot Invalid_enums.txt

wenovus commented 4 years ago

@robshakir I believe this is intentional in order to give 0 to UNSET? There is no need for actually using the values since the name can be used?

robshakir commented 4 years ago

I believe that we do not respect the value that is used in the YANG file - and generally use the 0 value to be default (UNSET if there is no specified default).

We could make this change - but it'll need to be flag-protected again since it'll change the validity of existing generated protos.