Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Annotation to integrate with Results #1245

Merged

Conversation

ramessesii2
Copy link
Member

@ramessesii2 ramessesii2 commented Apr 20, 2023

Changes

Purpose:
Feature request to add annotations to Pipelineruns produced by PaC. These annotations will be utilized in results to capture data for both summary and record.

UseCase:
Tekton results will store these fields in Summary and Record as Annotations. By default, Pipelinerun metadata information is JSON + base64 encoded. And in pipeline list APIs, we don't have information on what triggered the pipelines.
This field will be utilized by Dashboard/CLI clients to show the relevant data.

Fixes: #1234

Submitter Checklist

  • ♽ Run make test lint before submitting a PR (ie: with pre-commit, no need to waste CPU cycle on CI
  • 📖 If you are adding a user facing feature or make a change of the behavior, please verify that you have documented it
  • 🧪 100% coverage is not a target but most of the time we would rather have a unit test if you make a code change.
  • 🎁 If that's something that is possible to do please ensure to check if we can add a e2e test.
  • 🔎 If there is a flakiness in the CI tests then don't necessary ignore it, better get the flakyness fixed before merging or if that's not possible there is a good reason to bypass it. (token rate limitation may be a good reason to skip).

@ramessesii2
Copy link
Member Author

@khrm please take a look and let me know if you'd like anything else to be incorporated.

@pipelines-as-code
Copy link

pipelines-as-code bot commented Apr 20, 2023

Golang test coverage difference report

Coverage decreased by 0.05%. 🔔 Shame 🔔

Package report
package                                                                            before    after    delta
-------                                                                           -------  -------  -------
pkg/acl                                                                           100.00%  100.00%         
pkg/action                                                                         68.75%   68.75%         
pkg/adapter                                                                        72.41%   72.41%         
pkg/apis/features                                                                 100.00%  100.00%         
pkg/cli/info                                                                       88.23%   88.23%         
pkg/cli/prompt                                                                     55.38%   55.38%         
pkg/cli/status                                                                     95.23%   95.23%         
pkg/cli/webhook                                                                    49.40%   49.40%         
pkg/cmd/tknpac/bootstrap                                                            2.11%    2.11%         
pkg/cmd/tknpac/completion                                                          50.00%   50.00%         
pkg/cmd/tknpac/create                                                              44.14%   44.14%         
pkg/cmd/tknpac/describe                                                            46.31%   46.31%         
pkg/cmd/tknpac/generate                                                            62.20%   62.20%         
pkg/cmd/tknpac/info                                                                62.50%   62.50%         
pkg/cmd/tknpac/list                                                                46.47%   46.47%         
pkg/cmd/tknpac/resolve                                                             71.42%   71.42%         
pkg/cmd/tknpac/webhook                                                             52.47%   52.47%         
pkg/consoleui                                                                      82.60%   82.60%         
pkg/events                                                                         73.33%   73.33%         
pkg/formatting                                                                     98.71%   98.71%         
pkg/git                                                                            84.84%   84.84%         
pkg/hub                                                                            90.00%   90.00%         
pkg/kubeinteraction                                                                56.52%   52.50%   -4.02%
pkg/kubeinteraction/status                                                         77.27%   77.27%         
pkg/matcher                                                                        86.47%   86.47%         
pkg/params/clients                                                                 14.86%   14.86%         
pkg/params/settings                                                                79.48%   79.48%         
pkg/pipelineascode                                                                 64.95%   64.95%         
pkg/provider                                                                       76.19%   76.19%         
pkg/provider/bitbucketcloud                                                        87.09%   87.09%         
pkg/provider/bitbucketserver                                                       88.88%   88.88%         
pkg/provider/gitea                                                                 34.14%   34.14%         
pkg/provider/github                                                                82.05%   82.05%         
pkg/provider/github/app                                                            78.33%   78.33%         
pkg/provider/gitlab                                                                86.64%   86.64%         
pkg/random                                                                        100.00%  100.00%         
pkg/reconciler                                                                     42.19%   42.19%         
pkg/resolve                                                                        87.93%   87.93%         
pkg/secrets                                                                        92.85%   92.85%         
pkg/sort                                                                           51.20%   50.60%   -0.60%
pkg/sync                                                                           91.13%   91.13%         
pkg/templates                                                                     100.00%  100.00%         
pkg/webhook                                                                        22.22%   22.22%         
                                                                          total:   65.30%   65.25%   -0.05%

@chmouel
Copy link
Member

chmouel commented Apr 20, 2023

there is a e2e test to update as well to make sure the annotation was added and matches the sha properly
and please update the doc, feel free to use chat gpt or someone from the doc team to help for the phrasing,

it would be in the statuses documentation page, a section about results and how to use paac with it.

@ramessesii2
Copy link
Member Author

Thanks for the pointers @chmouel, I'll push the suggested changes soon.

@ramessesii2 ramessesii2 force-pushed the RAMESSESII2/results-annotation branch from 1e21808 to 1fe5583 Compare April 25, 2023 07:05
@ramessesii2 ramessesii2 changed the title [DNM] Add Annotation to integrate with Results [WIP] Add Annotation to integrate with Results Apr 25, 2023
},
{
name: "Empty Event",
event: &info.Event{},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if event is empty then accessing attributes of event such as event.SHA will fail right 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fields of the event that is being used for creating ResultsAnnotation should have default values in case it's an empty struct, right? I think that's why the unit tests pass.

@ramessesii2
Copy link
Member Author

ramessesii2 commented May 8, 2023

Quick Update: I'll try pushing the requested changes this week 🙃 .

@chmouel chmouel force-pushed the main branch 9 times, most recently from dc6b525 to 0488f72 Compare August 22, 2023 14:48
@ramessesii2 ramessesii2 force-pushed the RAMESSESII2/results-annotation branch from 1fe5583 to 10a5dcd Compare September 12, 2023 06:14
@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (b14dd80) 63.18% compared to head (2519df1) 63.12%.
Report is 3 commits behind head on main.

Files Patch % Lines
pkg/kubeinteraction/labels.go 33.33% 4 Missing and 2 partials ⚠️
pkg/pipelineascode/pipelineascode.go 16.66% 4 Missing and 1 partial ⚠️
pkg/kubeinteraction/resultsannotation.go 80.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1245      +/-   ##
==========================================
- Coverage   63.18%   63.12%   -0.06%     
==========================================
  Files         131      132       +1     
  Lines       10585    10606      +21     
==========================================
+ Hits         6688     6695       +7     
- Misses       3372     3382      +10     
- Partials      525      529       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ramessesii2 ramessesii2 force-pushed the RAMESSESII2/results-annotation branch 4 times, most recently from 7c42f0c to ce4e0d4 Compare September 17, 2023 15:44
@ramessesii2
Copy link
Member Author

ramessesii2 commented Sep 17, 2023

The final bits i.e. e2e & doc asked by @chmouel are added.
As of now, e2e test is failing, but the problem seems to be from Pipelines Webhook removing the annotation. One can see the following logs in the Pipelines Webhook : {"op":"remove","path":"/metadata/annotations/results.tekton.dev~1recordSummaryAnnotations"} which suggests that our annotation is being filtered out.

INFO  14:26:13 Kind: "tekton.dev/v1, Kind=PipelineRun" PatchBytes: [{"op":"add","path":"/metadata/creationTimestamp","value":null},{"op":"remove","path":"/metadata/annotations/results.tekton.dev~1recordSummaryAnnotations"},{"op":"add","path":"/spec/timeouts","value":{"pipeline":"1h0m0s"}},{"op":"add","path":"/spec/taskRunTemplate/serviceAccountName","value":"default"}]

INFO  14:26:13 Kind: "tekton.dev/v1, Kind=PipelineRun" PatchBytes: [{"op":"add","path":"/metadata/creationTimestamp","value":null},{"op":"remove","path":"/metadata/annotations/results.tekton.dev~1recordSummaryAnnotations"},{"op":"add","path":"/spec/taskRunTemplate/serviceAccountName","value":"default"},{"op":"add","path":"/spec/pipelineSpec/tasks/0/taskSpec/results/2/type","value":"string"},{"op":"add","path":"/spec/pipelineSpec/tasks/0/taskSpec/results/1/type","value":"string"},{"op":"add","path":"/spec/pipelineSpec/tasks/0/taskSpec/results/0/type","value":"string"},{"op":"add","path":"/spec/timeouts","value":{"pipeline":"1h0m0s"}}]

If I'm not wrong, this PR - tektoncd/pipeline#7108 by @khrm should give us a fix.

I can open this PR up for another review round if we're okay with it.

@ramessesii2 ramessesii2 force-pushed the RAMESSESII2/results-annotation branch from f41862e to 828b319 Compare December 11, 2023 13:06
@chmouel
Copy link
Member

chmouel commented Dec 11, 2023

LGTM but i let the others do a second pass on the review..

(and please squash your commits before merge)

@ramessesii2 ramessesii2 force-pushed the RAMESSESII2/results-annotation branch 2 times, most recently from a8f8278 to a6809a3 Compare December 11, 2023 13:58
@ramessesii2
Copy link
Member Author

Commits are squashed ✅
CI is green ✅

@ramessesii2 ramessesii2 force-pushed the RAMESSESII2/results-annotation branch 2 times, most recently from ab0fb30 to 8f97dae Compare December 20, 2023 10:00
@chmouel
Copy link
Member

chmouel commented Dec 20, 2023

LGTM

@chmouel
Copy link
Member

chmouel commented Dec 20, 2023

@khrm can you confirm me this is still needed? koustav told me this has been working without it....

@khrm
Copy link
Member

khrm commented Dec 20, 2023

It's not working without this. We manually added repo annotations to pipelines so that the dashboard can filtered by repo.

@khrm
Copy link
Member

khrm commented Dec 21, 2023

So Dashboard directly filters on labels set by PAC.

@chmouel
Copy link
Member

chmouel commented Dec 21, 2023

I am good to merge this! @openshift-pipelines/pipelines-as-code-contributors anything else to add ?

Satyam Bhardwaj added 2 commits December 29, 2023 05:28
Signed-off-by: Satyam Bhardwaj <[email protected]>
  - annotation :
    results.tekton.dev/recordSummaryAnnotations: |-
    {"repo": "tektoncd/results", "commit": "1a6b908",
    eventType: "pull_request",
    "pull_request-id": 6}
  - This annotation will be utilized in results to capture
    data.

Signed-off-by: Satyam Bhardwaj <[email protected]>
@ramessesii2 ramessesii2 force-pushed the RAMESSESII2/results-annotation branch from 8f97dae to 2519df1 Compare December 29, 2023 00:17
@ramessesii2 ramessesii2 requested a review from sm43 December 29, 2023 00:58
@chmouel
Copy link
Member

chmouel commented Jan 2, 2024

LGTM

@chmouel
Copy link
Member

chmouel commented Jan 2, 2024

/retest

@ramessesii2
Copy link
Member Author

The current CI failure might be unrelated to this PR!

task go-ninja-go has the status "Failed":

ok  	github.com/openshift-pipelines/pipelines-as-code/pkg/webhook	0.164s	coverage: 27.0% of statements
FAIL
make: *** [Makefile:60: test-unit] Error 1

/test go-testing

@chmouel chmouel merged commit 10bdd7c into openshift-pipelines:main Jan 2, 2024
4 checks passed
@chmouel
Copy link
Member

chmouel commented Jan 2, 2024

thanks you!

@ramessesii2 ramessesii2 deleted the RAMESSESII2/results-annotation branch January 2, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add Annotation to integrate with Results
6 participants