RSDK-6839 - Add machine part id to resource names

[Cheuk (cheukt)]

API change: viamrobotics/api#467

tried to limit scope of changes, in particular I didn't want to break the resource name's String() and FromString() method. Ultimately this means that the part id doesn't get serialized when converting to string/printed in logs, but don't think that's the worst. The alternative would mean we have to figure out a way to display the part id as part of the resource name in a string sanely, and I'm not sure that's possible.

So part IDs will be returned in clients, but all comparisons will be done without part IDs. I thought about adding a check to at least make sure that part ID == client's part ID, but didn't think it necessary since part ID is the same for all resources from a robot. Circumventing that doesn't seem to give any benefit but making the check strict may break code that works on multiple robots (a fleet that uses the same fragment, grab resource names from one robot and then trying to act on the whole fleet with those names). Let me know if you think the check would be valuable

[Cheuk (cheukt)]

will update once API changes are approved

[Cheuk (cheukt)]

flyby from an earlier version of changes that used inject.CloudMetadata a lot more

[Maxim Pertsov (maximpertsov)]

thanks!

[Benjamin Rewis (benjirewis)]

I made a comment in the API pr to ask why we're not also including the machine ID, but your reasoning about comparisons/stringifications sans part ID seem fine to me.

[Maxim Pertsov (maximpertsov)]

[q] will this be a *string since this field is optional?

i guess i'll have my answer once we merge the API change...

[Maxim Pertsov (maximpertsov)]

ah wait, you have the API changes locally - interesting, so unset optional string fields are still just blank strings?

[Cheuk (cheukt)]

it's a string

func (x *ResourceName) GetMachinePartId() string {
	if x != nil && x.MachinePartId != nil {
		return *x.MachinePartId
	}
	return ""
}

[Maxim Pertsov (maximpertsov)]

[q] do we have places were we might want to lookup or compare using either a name with OR without the part id?

If not, I think it would be helpful to have two different structs - one with the machine id and one without - e.g. Name, which includes the machine id, and NameWithoutMachineID, which does not (having the reverse is fine too - can probably be private/internal). This will prevent use from accidentally use a With/Without comparison when we meant to use the other one - it's also pretty easy to convert to the version we need.

---

[minor] Somewhat unrelated from my above comment - I slightly prefer we name these functions With/Without instead of Add/Remove, just to clarify that nothing is being mutated. We do document that behavior so no big deal if you prefer to leave as is.

Suggested change

	// AddPartID returns a new name with part ID added. This will replace an existing part ID.
	func (n Name) AddPartID(partID string) Name {
	tempName := NewName(n.API, n.ShortName())
	tempName.MachinePartID = partID
	return tempName
	}
	// RemovePartID returns a new name with part ID removed.
	func (n Name) RemovePartID() Name {
	return NewName(n.API, n.ShortName())
	}
	// WithPartID returns a new name with part ID added. This will replace an existing part ID.
	func (n Name) WithPartID(partID string) Name {
	tempName := NewName(n.API, n.ShortName())
	tempName.MachinePartID = partID
	return tempName
	}
	// WithoutPartID returns a new name with part ID removed.
	func (n Name) WithoutPartID() Name {
	return NewName(n.API, n.ShortName())
	}

[Cheuk (cheukt)]

I'll play around with an internal struct without machine id for usage around comparisons

[Cheuk (cheukt)]

playing around with it, I don't think we should make a separate struct.

A user should never use the struct without machine id, so this would be purely internal usage for internal structures that are tracking resources. If so, I would want to make it private, but that creates an import cycle if I want to embed NameWithoutMachineID back in Name. We could have resource.Name not embed NameWithoutMachineID, but that makes everything harder to maintain. Ultimately we just have to be vigilant around adding tests.

[Maxim Pertsov (maximpertsov)]

what about adding a "public" struct in the resource package but putting it in an internal folder so it can't be imported by external clients of rdk?

e.g.

// internal/resource/resource.go

package resource

type NameWithoutMachineID struct {
// ...
}

With this setup, we can import resource.NameWithoutMachineID wherever we need to, but external users won't have access to the this struct.

[Maxim Pertsov (maximpertsov)]

I guess this still has the downside of requiring a separate import e.g. go.viam.com/rdk/internal/resource. Anyway, I'm not gonna block on this - my general view is that we should use the static type system to make our lives easier whenever possible - so long as it's reasonable to achieve.

Thanks for exploring this.

[Cheuk (cheukt)]

my general view is that we should use the static type system to make our lives easier whenever possible - so long as it's reasonable to achieve.

ya, I agree with that, I do feel like this one is a bit difficult to achieve and ends up complicating the types a bit too much