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 Chains management command #1440

Merged
merged 3 commits into from
Mar 3, 2022
Merged

Add Chains management command #1440

merged 3 commits into from
Mar 3, 2022

Conversation

rgreinho
Copy link
Contributor

@rgreinho rgreinho commented Aug 27, 2021 •
edited
Loading

Changes

Adds a new sub-command to simplify performing some chains management
tasks like extracting the payload or the signature from a task run.

The goal of this PR is to get feedback about the idea of adding a new command and
sub-commands to perform some Tekton Chains operations. I believe it
is more user-friendly to use the CLI instead of a combination of kubectl and jq.

In this PR I simply added 2 sub-commands, but in the future we could envision adding
more, like for example changing the formatter (i.e. updating the Chains config map to
switch between "tekton-provenance" and "in-toto"), or verify the validity of the
signature.

Submitter Checklist

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

  • Includes tests (if functionality changed/added)
  • Run the code checkers with make check
  • Regenerate the manpages, docs and go formatting with make generated
  • Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 27, 2021
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 27, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

@tekton-robot
Copy link
Contributor

Hi @rgreinho. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 27, 2021
@rgreinho rgreinho force-pushed the chains-cli branch 2 times, most recently from d308c2d to 6cb6703 Compare August 27, 2021 19:05
@piyush-garg
Copy link
Contributor

@vdemeester @chmouel @pradeepitm12

Is not better to have a TEP for this new cmd?

@vdemeester
Copy link
Member

@vdemeester @chmouel @pradeepitm12

Is not better to have a TEP for this new cmd?

It would be nice discussing them there indeed 👼🏼

@rgreinho
Copy link
Contributor Author

Copy that! I am reading through the documentation to understand the process and I'll start writing a TEP about adding the new tkn sub-command for chains afterwards 👍

@trevrosen
Copy link

Nice proposal -- this would be fantastic to have!

@rgreinho
Copy link
Contributor Author

rgreinho commented Sep 2, 2021

Here is my TEP related to this change: tektoncd/community#508.

Looking forward to getting any feedback/remarks/critics/suggestions.

@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 23, 2021
@rgreinho rgreinho mentioned this pull request Oct 8, 2021
Merged
@tekton-robot tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 11, 2021
This was referenced Oct 11, 2021
Closed
Closed
@vdemeester
Copy link
Member

/retest

@rgreinho rgreinho marked this pull request as ready for review October 22, 2021 00:28
@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 4, 2022
@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2022
piyush-garg
piyush-garg reviewed Mar 1, 2022
View reviewed changes
pkg/cmd/chains/chains.go
)

const (
Namespace string = "tekton-chains"
Copy link
Contributor

Choose a reason for hiding this comment

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

This hardcoding makes it bit worrying as the namespace can be tekton-pipelines and openshift-pipelines if installed through operator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even for chains? So maybe we should use the namespace flag from the CLI and default to "tekton-chains" if it is not supplied?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any downside of installing chains in other namespaces? We are currently testing it in operator

Copy link
Contributor

Choose a reason for hiding this comment

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

Where can one see the progress of adding chains to the tekton operator? Also is there any details on when it would be added to OpenShift pipelines or nothing yet?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the PR on chains tektoncd/operator#630 It is targeted for upcoming releases i.e. Tekton Operator v0.55 and OpenShift Pipelines v1.7.0

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks

@rgreinho
Add Chains management command
9756fa6 
Adds a new sub-command to simplify performing some `chains` management
tasks like extracting the payload or the signature from a task run.

* Adds the ability to skip verifying the payload
* Adds the format sub-command
* Updates dependencies
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2022
@rgreinho
Copy link
Contributor Author

rgreinho commented Mar 1, 2022

There are a lot of errors since go mod vendor fails.

github.com/tektoncd/cli/cmd/tkn imports
	k8s.io/client-go/plugin/pkg/client/auth: package k8s.io/client-go/plugin/pkg/client/auth provided by k8s.io/client-go at latest version v0.23.4 but not at required version v1.5.2
github.com/tektoncd/cli/pkg/actions imports
	k8s.io/client-go/discovery: package k8s.io/client-go/discovery provided by k8s.io/client-go at latest version v0.23.4 but not at required version v1.5.2
github.com/tektoncd/cli/pkg/actions imports
	k8s.io/client-go/dynamic: package k8s.io/client-go/dynamic provided by k8s.io/client-go at latest version v0.23.4 but not at required version v1.5.2
github.com/tektoncd/cli/pkg/actions imports
	k8s.io/client-go/restmapper: package k8s.io/client-go/restmapper provided by k8s.io/client-go at latest version v0.23.4 but not at required version v1.5.2
github.com/tektoncd/cli/pkg/cli imports
	k8s.io/client-go/kubernetes: package k8s.io/client-go/kubernetes provided by k8s.io/client-go at latest version v0.23.4 but not at required version v1.5.2
github.com/tektoncd/cli/pkg/cli imports
[...]

Could I get some help fixing it please?

@piyush-garg
Copy link
Contributor

There are a lot of errors since go mod vendor fails.

github.com/tektoncd/cli/cmd/tkn imports
	k8s.io/client-go/plugin/pkg/client/auth: package k8s.io/client-go/plugin/pkg/client/auth provided by k8s.io/client-go at latest version v0.23.4 but not at required version v1.5.2
github.com/tektoncd/cli/pkg/actions imports
	k8s.io/client-go/discovery: package k8s.io/client-go/discovery provided by k8s.io/client-go at latest version v0.23.4 but not at required version v1.5.2
github.com/tektoncd/cli/pkg/actions imports
	k8s.io/client-go/dynamic: package k8s.io/client-go/dynamic provided by k8s.io/client-go at latest version v0.23.4 but not at required version v1.5.2
github.com/tektoncd/cli/pkg/actions imports
	k8s.io/client-go/restmapper: package k8s.io/client-go/restmapper provided by k8s.io/client-go at latest version v0.23.4 but not at required version v1.5.2
github.com/tektoncd/cli/pkg/cli imports
	k8s.io/client-go/kubernetes: package k8s.io/client-go/kubernetes provided by k8s.io/client-go at latest version v0.23.4 but not at required version v1.5.2
github.com/tektoncd/cli/pkg/cli imports
[...]

Could I get some help fixing it please?

Keep the replace function as it is like here https://github.com/tektoncd/cli/blob/main/go.mod#L44

and add the kube-api one inside it

@vdemeester
Copy link
Member

@rgreinho fixed the vendor problem, I'll carry this to get it merge quickly 👼🏼 😉

@tekton-robot
Copy link
Contributor

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

File Old Coverage New Coverage Delta
pkg/cmd/chains/chains.go Do not exist 0.0%
pkg/cmd/chains/payload.go Do not exist 0.0%
pkg/cmd/chains/signature.go Do not exist 0.0%

@tekton-robot
Copy link
Contributor

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

File Old Coverage New Coverage Delta
pkg/cmd/chains/chains.go Do not exist 0.0%
pkg/cmd/chains/payload.go Do not exist 0.0%
pkg/cmd/chains/signature.go Do not exist 0.0%

@tekton-robot
Copy link
Contributor

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

File Old Coverage New Coverage Delta
pkg/cmd/chains/chains.go Do not exist 0.0%
pkg/cmd/chains/payload.go Do not exist 0.0%
pkg/cmd/chains/signature.go Do not exist 0.0%

@vdemeester
Fix vendoring
9d3a8ca 
Signed-off-by: Vincent Demeester <[email protected]>
@tekton-robot
Copy link
Contributor

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

File Old Coverage New Coverage Delta
pkg/cmd/chains/chains.go Do not exist 0.0%
pkg/cmd/chains/payload.go Do not exist 0.0%
pkg/cmd/chains/signature.go Do not exist 0.0%

@tekton-robot
Copy link
Contributor

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

File Old Coverage New Coverage Delta
pkg/cmd/chains/chains.go Do not exist 0.0%
pkg/cmd/chains/payload.go Do not exist 0.0%
pkg/cmd/chains/signature.go Do not exist 0.0%

@vdemeester
Copy link
Member

/retest

@vdemeester
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2022
@tekton-robot tekton-robot merged commit 75b5e73 into tektoncd:main Mar 3, 2022
@rgreinho rgreinho deleted the chains-cli branch March 3, 2022 15:55
piyush-garg added a commit that referenced this pull request Mar 9, 2022
@piyush-garg
New version v0.23.0
ec7cf5e 
#1498 | [Pradeep Kumar] update tkn v0.22.0 | 2022/02/01-22:04
#1499 | [Daniel Helfand] update tkn chocolatey package to v0.22.0 | 2022/02/02-10:17
#1503 | [Chmouel Boudjnah] Use ubuntu-rolling to build package | 2022/02/03-10:17
#1505 | [Shubham] Make `vendor` a PHONY target | 2022/02/07-10:50
#1500 | [Chmouel Boudjnah] Add export option for pipelinerun | 2022/02/15-12:01
#1484 | [mansi103] Checked existence of resources before deletion | 2022/02/16-13:46
#1513 | [tomonight] pass rest config when create tkn clients,fix the default QPS is to low(QPS is 5),can improve the concurrent | 2022/02/28-09:14
#1514 | [Piyush Garg] Bump deps of pipeline and triggers | 2022/02/28-13:53
#1509 | [mansi103] Adds chains version in output of tkn version command | 2022/03/01-12:52
#1440 | [Rémy Greinhofer] Add Chains management command | 2022/03/03-15:01
null | [Vincent Demeester] Fix vendoring | 2022/03/03-15:01
null | [Vincent Demeester] Temporary disable linting tarball package | 2022/03/03-15:01
null | [jbpratt] remove unnecessary filepath join | 2022/03/04-08:13
null | [jbpratt] gofmt -w test/e2e/* | 2022/03/04-08:27
null | [jbpratt] replace t.Errorf %w with %v | 2022/03/04-09:17
null | [PuneetPunamiya] Bump Tekton Hub CLI to latest | 2022/03/04-10:37
null | [Pradeep Kumar] fix missing Cursor with interactive start commands | 2022/03/04-12:13
null | [Chmouel Boudjnah] Fix tektoncd docs installation instructions | 2022/03/08-08:08
null | [Piyush Garg] Make chains namespace configurable | 2022/03/09-09:29
null | [Piyush Garg] Bump chains and hub deps | 2022/03/09-09:29

Signed-off-by: Piyush Garg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@06kellyjac 06kellyjac 06kellyjac left review comments

@piyush-garg piyush-garg piyush-garg left review comments

@vdemeester vdemeester vdemeester approved these changes

@hrishin hrishin Awaiting requested review from hrishin

@pradeepitm12 pradeepitm12 Awaiting requested review from pradeepitm12

Assignees

@vdemeester vdemeester

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rgreinho @tekton-robot @piyush-garg @vdemeester @trevrosen @pradeepitm12 @06kellyjac