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.

Sign up for GitHub

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

Jump to bottom

Add apiVersion to trustedResources helper #6815

Closed
wants to merge 1 commit into from
Closed

Add apiVersion to trustedResources helper #6815

wants to merge 1 commit into from

Conversation

JeromeJu
Copy link
Member

Changes

This commit adds the apiVersion to the trustedResources helper which now allows the getSignedTask and etc helpers to return verified CRDs with the accepted apiVersions. This could help avoid confusions when we are adding v1 CRDs to be verified in the test cases.

/kind misc

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
  • Has Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including functionality, content, code)
  • Has a kind label. You can add one by adding 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). See some examples of good release notes.
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/misc Categorizes issue or PR as a miscellaneuous one. labels Jun 12, 2023
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 12, 2023
@JeromeJu
Copy link
Member Author

cc @lbernick @Yongxuanzhang

@JeromeJu JeromeJu mentioned this pull request Jun 12, 2023
Closed
4 tasks
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 9.0% -1.6

@JeromeJu JeromeJu force-pushed the apiversioned-trustedresources branch from be24f90 to 2517acd Compare June 12, 2023 14:55
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 9.0% -1.6

@JeromeJu JeromeJu mentioned this pull request Jun 12, 2023
Merged
7 tasks
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 9.0% -1.6

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 9.0% -1.6

@JeromeJu JeromeJu force-pushed the apiversioned-trustedresources branch from 2517acd to 1d346dd Compare June 12, 2023 15:05
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 9.0% -1.6

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 9.0% -1.6

@lbernick lbernick self-assigned this Jun 12, 2023
@JeromeJu JeromeJu force-pushed the apiversioned-trustedresources branch from 1d346dd to 9a12ab9 Compare June 12, 2023 15:33
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from lbernick after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

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

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 9.0% -1.6

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 9.0% -1.6

Yongxuanzhang
Yongxuanzhang reviewed Jun 12, 2023
View reviewed changes
Copy link
Member

@Yongxuanzhang Yongxuanzhang left a comment

Choose a reason for hiding this comment

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

I think you don't need to change the getunsigned helpers since they will be removed in #6800 and #6799

test/trustedresources.go Outdated Show resolved Hide resolved
test/trusted_resources_test.go Outdated Show resolved Hide resolved
test/trustedresources.go
if err != nil {
return nil, err
// GetSignedPipeline signed the given pipeline and rename it with given name and apiVersion
func GetSignedPipeline(unsigned runtime.Object, signer signature.Signer, name, apiVersion string) (runtime.Object, error) {
Copy link
Member

Choose a reason for hiding this comment

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

one disadvantage of merging 2 helper functions is that we need to do type assert for the returned obj

@JeromeJu JeromeJu force-pushed the apiversioned-trustedresources branch from 9a12ab9 to d661c7d Compare June 12, 2023 20:19
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 8.6% -2.0

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 8.6% -2.0

@JeromeJu JeromeJu force-pushed the apiversioned-trustedresources branch from d661c7d to 29f1138 Compare June 12, 2023 20:58
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 8.6% -2.0

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 8.6% -2.0

@JeromeJu JeromeJu force-pushed the apiversioned-trustedresources branch from 29f1138 to a306cd4 Compare June 12, 2023 21:14
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 10.2% -0.4

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 10.2% -0.4

@JeromeJu JeromeJu force-pushed the apiversioned-trustedresources branch from a306cd4 to fe742de Compare June 13, 2023 18:44
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 10.2% -0.4

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 10.2% -0.4

lbernick
lbernick reviewed Jun 14, 2023
View reviewed changes
pkg/reconciler/pipelinerun/pipelinerun_test.go Outdated Show resolved Hide resolved
pkg/reconciler/pipelinerun/pipelinerun_test.go Outdated Show resolved Hide resolved
pkg/reconciler/pipelinerun/resources/pipelineref_test.go Show resolved Hide resolved
pkg/trustedresources/verify_test.go Outdated Show resolved Hide resolved
@JeromeJu JeromeJu force-pushed the apiversioned-trustedresources branch from fe742de to 34889b0 Compare June 15, 2023 13:58
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 8.6% -2.0

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 10.6% 8.6% -2.0

Yongxuanzhang
Yongxuanzhang reviewed Jun 15, 2023
View reviewed changes
Copy link
Member

@Yongxuanzhang Yongxuanzhang left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 15, 2023
@JeromeJu
Add apiVersion to trustedResources helper
fba9a5a 
This commit adds the apiVersion to the trustedResources helper which
now allows the getSignedTask and etc helpers to return verified CRDs
with the accepted apiVersions. This could help avoid confusions when we
are adding v1 CRDs to be verified in the test cases.
@JeromeJu JeromeJu force-pushed the apiversioned-trustedresources branch from 34889b0 to fba9a5a Compare June 19, 2023 20:52
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2023
@tekton-robot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2023
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 8.9% 7.5% -1.5

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
test/trustedresources.go 8.9% 7.5% -1.5

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2023
@tekton-robot
Copy link
Collaborator

@JeromeJu: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 31, 2023
@afrittoli
Copy link
Member

Hello @JeromeJu - is this something you're still pursuing?

@JeromeJu JeromeJu closed this Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbernick lbernick lbernick left review comments

@Yongxuanzhang Yongxuanzhang Yongxuanzhang left review comments

@pritidesai pritidesai Awaiting requested review from pritidesai

@vdemeester vdemeester Awaiting requested review from vdemeester

Assignees

@lbernick lbernick

@Yongxuanzhang Yongxuanzhang

Labels
kind/misc Categorizes issue or PR as a miscellaneuous one. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesnt merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JeromeJu @tekton-robot @afrittoli @lbernick @Yongxuanzhang