Update all call-sites to ensure ResourceRefs use cluster.

[absoludity]

Signed-off-by: Michael Nelson minelson@vmware.com

Description of the change

Follows on from #1886, updating ResourceRefs to include the cluster (in the constructor as a required arg) then updating all the call-sites to use it (mainly in tests, other than AppView and OperatorInstance (which uses fromCRD)).

It was a pain to audit because the typechecker can't find everything, as the 3rd arg is an optional string. Anyway, I think I've audited all call-sites now.

This is slightly different to how I did the change in the demo branch, and has the benefit that the resource refs just calculate multicluster URLs once created, so users of the objects don't need to care.

Benefits

Required for multicluster support.

Possible drawbacks

Danger of missing a call-site.

Applicable issues

Ref: #1762

Additional information

[andresmgot]

Thanks! Mostly LGTM, let me know what you think about what I propose.

[andresmgot]

for the maintainability of this I would:

• Make the defaultNamespace mandatory. It's easier to understand calls like new ResourceRef({}, "cluster-a", namespace). Also you would notice if you are missing the cluster in some call.
• Remove the defaultFilter param. AFAIK, it's only used in the function fromCRD above. You can move it to a setter.

[absoludity]

for the maintainability of this I would:

These are both not things I changed, but always keen to improve things...

* Make the `defaultNamespace` mandatory. It's easier to understand calls like `new ResourceRef({}, "cluster-a", namespace)`. Also you would notice if you are missing the cluster in some call.

It's not clear to me why we even have this arg... the docstring says in case an IResource doesn't have a namespace, but it's a required field on IResource. I think we are only ever showing namespaced resources? Smells either like a hack to work-around bad input, or old cruft that hasn't been removed? Let me know, but either way I'm keen to change that in a separate branch and move forward here.

* Remove the `defaultFilter` param. AFAIK, it's only used in the function `fromCRD` above. You can move it to a setter.

+1. It's a public field, so just set it in fromCRD in aa48611

[andresmgot]

It's not clear to me why we even have this arg... the docstring says in case an IResource doesn't have a namespace, but it's a required field on IResource. I think we are only ever showing namespaced resources? Smells either like a hack to work-around bad input, or old cruft that hasn't been removed? Let me know, but either way I'm keen to change that in a separate branch and move forward here.

This parameter is needed because we parse the manifests installed by helm to retrieve the different resources. With helm, it's not necessary to set the namespace. If a resource doesn't have a namespace, it's installed in the namespace of the release. Also (I am not sure about this), if the resource is a list of resources, the namespaces may not be specified in each of those elements so we would need to pass the namespace of the list.

[absoludity]

Thanks for the explanation. That being the case, it seems odd to require it as a default even if it's not necessary (rather than just leaving it as an optional default). But I'll take a closer look.