cartographer icon indicating copy to clipboard operation
cartographer copied to clipboard

RFC: Add matchParams selector

Open jwntrs opened this issue 3 years ago • 8 comments

Add matchParams selector Readable

jwntrs avatar Feb 14 '22 17:02 jwntrs

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

🔨 Explore the source changes: c557c5d7291b3801941cc7e95d5deb16f1005a1b

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

netlify[bot] avatar Feb 14 '22 17:02 netlify[bot]

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?

jwntrs avatar Feb 15 '22 15:02 jwntrs

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.

sclevine avatar Feb 15 '22 16:02 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.

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 avatar Feb 16 '22 14:02 squeedee

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.

scothis avatar Feb 16 '22 14:02 scothis

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.

waciumawanjohi avatar Feb 16 '22 15:02 waciumawanjohi

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.

martyspiewak avatar Feb 16 '22 15:02 martyspiewak

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.

jwntrs avatar Feb 25 '22 16:02 jwntrs