Add ID fields to Resource Objects for easy lookup and validations #481
Conversation
lib/api/bgppeer.go
Outdated
| @@ -37,6 +37,10 @@ func (t BGPPeer) GetResourceMetadata() unversioned.ResourceMetadata { | |||
| return t.Metadata | |||
| } | |||
|
|
|||
| func (t BGPPeer) GetResourceID() string { | |||
There was a problem hiding this comment.
I think you're better off having this method return a user friendly name that identifies this resource. That way it could be returned to the user as well. e.g. "BGP Peer (PeerIP=xxx, Scope=Global)"
lib/validator/common.go
Outdated
| @@ -80,3 +82,39 @@ func ValidateMetadataIDsAssigned(rm unversioned.ResourceMetadata) error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| // ValidateIDFieldsSame is used to validate that the Resource Objects | |||
There was a problem hiding this comment.
I think this is only useful for calicoctl, so I'd not include this in libcalico-go.
lib/validator/common.go
Outdated
| // GetIDFields is a helper function that returns the field labels | ||
| // on the metadata objects that are ID fields excluding Labels, | ||
| // Annotations, and Tags | ||
| func GetIDFields(rm unversioned.ResourceMetadata) []string { |
There was a problem hiding this comment.
I don't like this. I think if you really want to get a slice of IDs as strings, then the getResourceID() should probably have been getResourceIDs() returning a slice of strings.
Also, if you are going to go to the trouble of reading the tags, then it would be better to add a new tag type to identify the field as an ID. Skipping over some types and assuming the rest are IDs isn't very maintainable if we start padding out the Metadata with other fields.
Personally I don't think that's necessary - if you have a method that returns a nice user friendly name of the resource then you can craft some text that says Resource X should be Resource Y.
There was a problem hiding this comment.
I see. My thought was to basically write a function that would work for all the resources without needing to create more individual methods on each that would be need to be maintained should something change. To that end, adding a new tag type would probably be best as far as that line of thinking goes. You're right though; all of this is probably unnecessary if I can get a user friendly name working so then I can parse those values. WorkloadEndpoint might have a little bit of a long string name though....
lib/api/bgppeer.go
Outdated
| @@ -37,6 +39,10 @@ func (t BGPPeer) GetResourceMetadata() unversioned.ResourceMetadata { | |||
| return t.Metadata | |||
| } | |||
|
|
|||
| func (t BGPPeer) String() string { | |||
| return fmt.Sprintf("BGPPeer(IP=%s, Scope=%s, Node=%s)", t.Metadata.PeerIP.IP.String(), t.Metadata.Scope, t.Metadata.Node) | |||
There was a problem hiding this comment.
Not too fussed about this, but perhaps if scope is node and node=="" we could omit node from the name.
lib/api/unversioned/types.go
Outdated
| @@ -23,6 +23,7 @@ type Resource interface { | |||
| type ResourceObject interface { | |||
| Resource | |||
| GetResourceMetadata() ResourceMetadata | |||
| String() string | |||
There was a problem hiding this comment.
Given the very specific behavior of this String() method, I think it is worth adding a doc comment.
lib/api/workloadendpoint.go
Outdated
| @@ -31,6 +31,10 @@ func (t WorkloadEndpoint) GetResourceMetadata() unversioned.ResourceMetadata { | |||
| return t.Metadata | |||
| } | |||
|
|
|||
| func (t WorkloadEndpoint) String() string { | |||
| return fmt.Sprintf("WorkloadEndpoint(Node=%s, Orchestrator=%s, Workload=%s, Name=%s, ActiveInstanceID=%s)", t.Metadata.Node, t.Metadata.Orchestrator, t.Metadata.Workload, t.Metadata.Name, t.Metadata.ActiveInstanceID) | |||
There was a problem hiding this comment.
I think we can remove the Active Instance ID, or perhaps only include it if it is set to something - it's used under very specific circumstances.
Description
Adds in an ID field for every Resource Object and validations on top of those ID fields to make it easier to validate that important fields were not changed.
Will be of help to finishing projectcalico/calicoctl#1564
Todos
Release Note