RFC: Add matchParams selector

[jwntrs]

Add matchParams selector
Readable

[netlify]

✔️ Deploy Preview for elated-stonebraker-105904 canceled.

🔨 Explore the source changes: c557c5d

🔍 Inspect the deploy log: https://app.netlify.com/sites/elated-stonebraker-105904/deploys/621507c2195d720008057add

[scothis]

Since matchFields is generic and able to match any field within the resource, I'd rather not overload that syntax by creating shortcuts for params. Even moreso if we are looking to change the target of the match from the workload/deliverable definition to values that are defined other places like on the ClusterSupplyChain/ClusterDeliverable or at runtime.

If we think that params are important enough to match on, they deserve top level support in a matchParams facet of the selector.

[jwntrs]

If we think that params are important enough to match on, they deserve top level support in a matchParams facet of the selector.

@scothis yeah @emmjohnson suggested something similar in our community meeting. I'm happy to take that direction with this RFC. @sclevine I think you mentioned in the meeting that you preferred the RFC as written, any objections to making the above change?

[sclevine]

Since matchFields is generic and able to match any field within the resource, I'd rather not overload that syntax by creating shortcuts for params.

@scothis We already have this shortcut in other places, and matchFields already uses non-standard notation for the field selector path (e.g., workload.spec instead spec). I suspect uses will assume that params works with matchFields, just as it works in other places. But no strong opinion.

[squeedee]

Since matchFields is generic and able to match any field within the resource, I'd rather not overload that syntax by creating shortcuts for params.

Despite originally thinking that MatchParams was overkill, I think you're right. MatchFields has a clear context, and Params are the resolution of partial-tree. I think drawing a user's attention to this special case with this separate selector is more educational.

[squeedee]

I think this rfc is important in it's ability to provide defaultable fields.

The secondary, "supply chain configuration at install time" might actually be an anti-pattern? Creating a hard coded but 'selected' path will present a strange UX for visualiser, whereas templating at install with wholesale replacement of a resoruce makes for clearer supply chains.

[scothis]

I think this rfc is important in it's ability to provide defaultable fields.

That's a compelling distinction as it is not possible with a vanilla field selector operating against the Workload structure directly. It requires a partial templating context to match against.

I'm still on the fence as to whether matchFields would be better actually as a vanilla field selector operating against the Workload structure directly and the new behavior be defined within a matchParam facet.

[waciumawanjohi]

This strikes me as a bit of syntactic sugar. Params can be set in 4 places:

• In the template
• In the resource field of a supply chain
• At the top level of a supply chain
• In the workload

When specifying a MatchParam, the supply-chain author knows 3 of these values already. Ultimately they are just checking, has the supply chain overwritten the choices made at the other 3 levels. So they need only check if the param exists on the workload and if it has a given value.

I have no objections to the change, but I wonder at its utility.

[martyspiewak]

I think I agree with @emmjohnson and @scothis that if we're going to inject the param hierarchy logic in this field, it deserves its own matchParams key to be explicit.

But as @waciumawanjohi said, I struggle to understand the practical use case for this.

[waciumawanjohi]

In this example, the workload param would be ignored, as the value has been hard coded by the supply-chain author. Switch value to default to clarify that the workload param is allowed.

Suggested change

	value: (gitops|regops)
	default: (gitops|regops)

[jwntrs]

But as @waciumawanjohi said, I struggle to understand the practical use case for this.

@martyspiewak @waciumawanjohi as @squeedee pointed out this is about providing a path for defaultable fields. I've updated the yaml in the RFC to better illustrate this use case.

[squeedee]

When specifying a MatchParam, the supply-chain author knows 3 of these values already. Ultimately they are just checking, has the supply chain overwritten the choices made at the other 3 levels. So they need only check if the param exists on the workload and if it has a given value.

I have no objections to the change, but I wonder at its utility.

I agree with @waciumawanjohi and @martyspiewak here, this is well reasoned. Adding this feature might reduce a "change in two places" complexity, however as it just adds complexity to the code and we've not had any user reports for this, I vote to move this to declined, with a note that it should be reopened should strong user need arise.