Allow users/integrators to pass arbitrary keys/values through Result and RecordSummary annotations #426

alan-ghelardi · 2023-04-03T15:23:07Z

Changes

Resolves #343.

Users can pass arbitrary keys/values to the Result.Annotations and
Result.Summary.Annotations fields by adding certain annotations to their
PipelineRuns and TaskRuns.

/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

Allow users/integrators to pass arbitrary keys/values to the Result.Annotations
and Result.Summary.Annotations fields, by adding the newly created annotations
results.tekton.dev/resultAnnotations and
results.tekton.dev/recordSummaryAnnotations to their PipelineRuns and TaskRuns.

Those annotations can be used to store relevant metadata (e.g. repository name,
Git commit SHA, pull request number among others) along with results into the
database, in order to be used later to retrieve those resources via the API.

alan-ghelardi · 2023-04-03T15:27:12Z

/kind feature

khrm

/lgtm

alan-ghelardi · 2023-04-03T19:43:09Z

/assign @khrm
/assign @adambkaplan
/assign @dibyom

adambkaplan

@alan-ghelardi can you please add markdown documentation for this feature?

In general looks good, I just have one question on the implementation.

adambkaplan · 2023-04-11T21:29:04Z

pkg/watcher/results/results.go

+		if annotations, err := parseAnnotations(annotation.ResultAnnotations, value); err != nil {
+			return nil, err
+		} else {
+			res.Annotations = annotations


Are there other annotations that we add to the Result record? Should we merge annotations instead of overwrite?

We are just copying these annotations from PR for LIST API. So no merge requires. If these annotation changes, then we require a new one. I think it's the responsibility of integrators to ensure that we always merge not overwrite these.

adambkaplan · 2023-04-11T21:29:19Z

pkg/watcher/results/results.go

+		if annotations, err := parseAnnotations(annotation.RecordSummaryAnnotations, value); err != nil {
+			return nil, err
+		} else if res.Summary != nil {
+			res.Summary.Annotations = annotations


Likewise - merge instead of overwrite?

adambkaplan · 2023-04-11T21:32:44Z

/approve

This feature can be useful for operators and integrators.

tekton-robot · 2023-04-11T21:32:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [adambkaplan]

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

dibyom · 2023-04-20T20:01:24Z

I have the same question as Adam around merging vs overwriting values but otherwise looks good. Thanks @alan-ghelardi

khrm · 2023-04-21T00:41:55Z

pkg/watcher/results/results.go

+			if res.Annotations == nil {
+				res.Annotations = make(map[string]string, len(annotations))
+			}
+			copyKeys(annotations, res.Annotations)


Isn't res.Annotations always empty? You should use curr.Annotations and copy it to res.Annotations if we want to merge.

If we don't want to employ merge, was there any issue with the previous approach of just copying?

We can just delegate the responsibility of ensuring that the merge happens to integrators. And we always do upsert.

Merging values made the code trickier, but ultimately, it seems a good idea thinking about future extensibility.

khrm · 2023-04-21T00:48:02Z

pkg/watcher/results/results.go

+		if annotations, err := parseAnnotations(annotation.RecordSummaryAnnotations, value); err != nil {
+			return nil, err
+		} else if res.Summary != nil {
+			if res.Summary.Annotations == nil {


Same issue here. We should first copy curr.Summary.Annotations to res.Summary.Annotations .

RecordSummary annotations.

alan-ghelardi · 2023-04-21T11:25:54Z

@adambkaplan and @dibyom in practice nowadays there aren't other entrypoints to set those annotations, but I like the idea of merging the values to support extensibility.

@tektoncd/results-maintainers could you have a second review here, please?

alan-ghelardi · 2023-04-21T11:27:06Z

I updated the docs with the new feature, by thee way.

dibyom · 2023-04-25T14:46:20Z

/lgtm

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 3, 2023

tekton-robot requested review from dibyom and XinruZhang April 3, 2023 15:23

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 3, 2023

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 3, 2023

alan-ghelardi added this to the Enhance the Results API search capabilities milestone Apr 3, 2023

khrm reviewed Apr 3, 2023

View reviewed changes

tekton-robot assigned khrm Apr 3, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2023

alan-ghelardi force-pushed the result-and-record-summary-annotations branch from db319d5 to 9ca4778 Compare April 3, 2023 19:39

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2023

tekton-robot assigned adambkaplan and dibyom Apr 3, 2023

alan-ghelardi mentioned this pull request Apr 5, 2023

Do not create the parent result if the object being reconciled is a child resource #339

Closed

adambkaplan reviewed Apr 11, 2023

View reviewed changes

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 11, 2023

alan-ghelardi force-pushed the result-and-record-summary-annotations branch from 9ca4778 to 02150f5 Compare April 20, 2023 22:21

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2023

alan-ghelardi force-pushed the result-and-record-summary-annotations branch from 02150f5 to f8d7707 Compare April 20, 2023 22:33

khrm reviewed Apr 21, 2023

View reviewed changes

alan-ghelardi force-pushed the result-and-record-summary-annotations branch from f8d7707 to 7fde7d9 Compare April 21, 2023 01:42

Allow users/integrators to pass arbitrary keys/values through Result and

65a2e33

RecordSummary annotations.

alan-ghelardi force-pushed the result-and-record-summary-annotations branch from 7fde7d9 to 65a2e33 Compare April 21, 2023 11:17

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2023

tekton-robot merged commit fdc4754 into tektoncd:main Apr 25, 2023

alan-ghelardi mentioned this pull request Apr 25, 2023

Paging with Pagetoken & Filter doesn't work properly #379

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users/integrators to pass arbitrary keys/values through Result and RecordSummary annotations #426

Allow users/integrators to pass arbitrary keys/values through Result and RecordSummary annotations #426

alan-ghelardi commented Apr 3, 2023 •

edited

Loading

alan-ghelardi commented Apr 3, 2023

khrm left a comment

alan-ghelardi commented Apr 3, 2023

adambkaplan left a comment

adambkaplan Apr 11, 2023

khrm Apr 19, 2023

adambkaplan Apr 11, 2023

adambkaplan commented Apr 11, 2023

tekton-robot commented Apr 11, 2023

dibyom commented Apr 20, 2023

khrm Apr 21, 2023 •

edited

Loading

khrm Apr 21, 2023 •

edited

Loading

alan-ghelardi Apr 21, 2023

khrm Apr 21, 2023

alan-ghelardi commented Apr 21, 2023

alan-ghelardi commented Apr 21, 2023

dibyom commented Apr 25, 2023

Allow users/integrators to pass arbitrary keys/values through Result and RecordSummary annotations #426

Allow users/integrators to pass arbitrary keys/values through Result and RecordSummary annotations #426

Conversation

alan-ghelardi commented Apr 3, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

alan-ghelardi commented Apr 3, 2023

khrm left a comment

Choose a reason for hiding this comment

alan-ghelardi commented Apr 3, 2023

adambkaplan left a comment

Choose a reason for hiding this comment

adambkaplan Apr 11, 2023

Choose a reason for hiding this comment

khrm Apr 19, 2023

Choose a reason for hiding this comment

adambkaplan Apr 11, 2023

Choose a reason for hiding this comment

adambkaplan commented Apr 11, 2023

tekton-robot commented Apr 11, 2023

dibyom commented Apr 20, 2023

khrm Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

khrm Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

alan-ghelardi Apr 21, 2023

Choose a reason for hiding this comment

khrm Apr 21, 2023

Choose a reason for hiding this comment

alan-ghelardi commented Apr 21, 2023

alan-ghelardi commented Apr 21, 2023

dibyom commented Apr 25, 2023

alan-ghelardi commented Apr 3, 2023 •

edited

Loading

khrm Apr 21, 2023 •

edited

Loading

khrm Apr 21, 2023 •

edited

Loading