feat(watcher): add --required_annotations to hold the finalizer until extra keys are true

[ankrsinha]

Changes

The watcher currently clears its finalizer as soon as
results.tekton.dev/stored is true. In multicluster setups,
external controllers may still need to annotate the PipelineRun or
TaskRun, and those writes are lost if the object is deleted first.

This PR adds a --required_annotations flag: a comma-separated list of
extra annotation keys that must all be set to "true" before the
finalizer is cleared.

• results.tekton.dev/stored stays required and does not need to be listed
• If the flag is empty (default), behavior is unchanged
• PipelineRun and TaskRun reconcilers requeue until every listed key is "true"
• store_deadline still clears the finalizer so objects cannot stay stuck
• CustomRuns are not affected

/kind feature

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you review them:

• Has Docs included if any changes are user facing
• Has Tests included if any functionality added or changed
• Tested your changes locally (if this is a code change)
• Follows the commit message standard
• Meets the Tekton contributor standards (including functionality, content, code)
• Has a kind label. You can add a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
• Release notes block below has been updated with any user-facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
• Release notes contain the string "action required" if the change requires additional action from users switching to the new release

Release Notes

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign divyansh42 after the PR has been reviewed.
You can assign the PR to them by writing /assign @divyansh42 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[enarha]

IMO the expectation that the value of annotation is always "true" (or "false") is optimistic and limits the design and would require breaking changes to extend. By definition, the value of annotation is a arbitrary string, so assume someone wants to filter on "color": "green" or "ci": "passed". This complicates the design because you can't properly parse the flag value if provided as a string as the annotation value can really include any char, including =, ,, :. One option is to ask the user to pass the --required_annotations flag multiple times e.g. --required_annotation "color:green" --required_annotation "ci:passed". This could work unambiguously because there are certain limitations on the annotation key, so you can split on the first : or first =, consider that the key and the rest of the string as the value. This is from the top of my head and it should be properly evaluated and other options can exist as well.

Another thing to consider is that today we decided to apply that filter to PipelineRun and TaskRun and exclude CustomRun. I understand why it's done now this way, but that's not really generic. We are planning to extend support in Tekton Results for arbitrary Kubernetes objects and I can imagine users asking for that feature for any type of stored object. I do not want to complicate the design too much, but if we can also achieve that from the start e.g. map a list of resources to a list of annotations, that would be better. We can go with the current design, but switching to new configuration design becomes harder later.

[ankrsinha]

IMO the expectation that the value of annotation is always "true" (or "false") is optimistic and limits the design and would require breaking changes to extend. By definition, the value of annotation is a arbitrary string, so assume someone wants to filter on "color": "green" or "ci": "passed". This complicates the design because you can't properly parse the flag value if provided as a string as the annotation value can really include any char, including =, ,, :. One option is to ask the user to pass the --required_annotations flag multiple times e.g. --required_annotation "color:green" --required_annotation "ci:passed". This could work unambiguously because there are certain limitations on the annotation key, so you can split on the first : or first =, consider that the key and the rest of the string as the value. This is from the top of my head and it should be properly evaluated and other options can exist as well.

@enarha Agreed on this. we can switch to a repeatable --required_annotation flag with key:value parsing. If no separator is given, the value defaults to "true", so the common case (checking for a boolean annotation) stays simple.

Ex-

--required_annotation "hub.example.com/scheduled"          # implies :true
--required_annotation "hub.example.com/scheduled:true"     # explicit
--required_annotation "ci.example.com/status:passed"       # arbitrary value

Implementation-wise, Go's flag.String() can't handle repeatable flags, so this would need a custom flag.Value type (AnnotationFlag), changing the []string to map[string]string, and updating the parsing + check logic accordingly.

wdyt @khrm ? Shall i go ahead and make changes for this ?

[enarha]

IMO the expectation that the value of annotation is always "true" (or "false") is optimistic and limits the design and would require breaking changes to extend. By definition, the value of annotation is a arbitrary string, so assume someone wants to filter on "color": "green" or "ci": "passed". This complicates the design because you can't properly parse the flag value if provided as a string as the annotation value can really include any char, including =, ,, :. One option is to ask the user to pass the --required_annotations flag multiple times e.g. --required_annotation "color:green" --required_annotation "ci:passed". This could work unambiguously because there are certain limitations on the annotation key, so you can split on the first : or first =, consider that the key and the rest of the string as the value. This is from the top of my head and it should be properly evaluated and other options can exist as well.

@enarha Agreed on this. we can switch to a repeatable --required_annotation flag with key:value parsing. If no separator is given, the value defaults to "true", so the common case (checking for a boolean annotation) stays simple.

Ex-

--required_annotation "hub.example.com/scheduled"          # implies :true
--required_annotation "hub.example.com/scheduled:true"     # explicit
--required_annotation "ci.example.com/status:passed"       # arbitrary value

Implementation-wise, Go's flag.String() can't handle repeatable flags, so this would need a custom flag.Value type (AnnotationFlag), changing the []string to map[string]string, and updating the parsing + check logic accordingly.

wdyt @khrm ? Shall i go ahead and make changes for this ?

Personally I'm not a huge fan of the implicit value "hub.example.com/scheduled" # implies :true. This should be relatively rare action to update the list of required annotations, so it wouldn't save much work from the user to be explicit here. It also makes our code simpler, one if branch less. That's IMO.

The other thing if which character to designate as separator, : or =. The former resembles better the actual yaml syntax, the latter is more readable IMO, both a fine to use in this case. You can have a look in existing tools (check few more popular ones) and check how they handle that and which character they prefer and that may help us decide which is better. Again, I do not have strong opinion, just want to make it more native for the users.

[enarha]

IMO the expectation that the value of annotation is always "true" (or "false") is optimistic and limits the design and would require breaking changes to extend. By definition, the value of annotation is a arbitrary string, so assume someone wants to filter on "color": "green" or "ci": "passed". This complicates the design because you can't properly parse the flag value if provided as a string as the annotation value can really include any char, including =, ,, :. One option is to ask the user to pass the --required_annotations flag multiple times e.g. --required_annotation "color:green" --required_annotation "ci:passed". This could work unambiguously because there are certain limitations on the annotation key, so you can split on the first : or first =, consider that the key and the rest of the string as the value. This is from the top of my head and it should be properly evaluated and other options can exist as well.

@enarha Agreed on this. we can switch to a repeatable --required_annotation flag with key:value parsing. If no separator is given, the value defaults to "true", so the common case (checking for a boolean annotation) stays simple.
Ex-

--required_annotation "hub.example.com/scheduled"          # implies :true
--required_annotation "hub.example.com/scheduled:true"     # explicit
--required_annotation "ci.example.com/status:passed"       # arbitrary value

Implementation-wise, Go's flag.String() can't handle repeatable flags, so this would need a custom flag.Value type (AnnotationFlag), changing the []string to map[string]string, and updating the parsing + check logic accordingly.
wdyt @khrm ? Shall i go ahead and make changes for this ?

Personally I'm not a huge fan of the implicit value "hub.example.com/scheduled" # implies :true. This should be relatively rare action to update the list of required annotations, so it wouldn't save much work from the user to be explicit here. It also makes our code simpler, one if branch less. That's IMO.

The other thing if which character to designate as separator, : or =. The former resembles better the actual yaml syntax, the latter is more readable IMO, both a fine to use in this case. You can have a look in existing tools (check few more popular ones) and check how they handle that and which character they prefer and that may help us decide which is better. Again, I do not have strong opinion, just want to make it more native for the users.

Quick search shows that = is more popular. For instance kubectl annotate pods foo description='my frontend'. There are exceptions, but the = is prevailing.