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

feat: add CustomRun commad with list #2213

Merged
merged 1 commit into from Jan 25, 2024
Merged

feat: add CustomRun commad with list #2213

merged 1 commit into from Jan 25, 2024

Conversation

stillfox-lee
Copy link
Contributor

Changes

Add CustomRun with list subcommand.
issue: #2195

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

Support CustomRun list subcommand.

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 15, 2024
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Jan 15, 2024 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: stillfox-lee / name: lsy (6edaa76)

@tekton-robot tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 15, 2024
@tekton-robot
Copy link
Contributor

Hi @stillfox-lee. 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 the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 15, 2024
@piyush-garg
Copy link
Contributor

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 15, 2024
@piyush-garg piyush-garg linked an issue Jan 15, 2024 that may be closed by this pull request
Support CustomRun list command. #2195
Closed
piyush-garg
piyush-garg reviewed Jan 16, 2024
View reviewed changes
docs/cmd/tkn_customrun.md

Manage CustomRuns

***Aliases**: cr,customruns*
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want cr? It can be confusing later because we will add customtask too, and then we can't have ct as it is for clustertask, so this will be a bit inconsistent.

I would say let's think of alias, which can work for both customrun and customtask

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @vinamra28 @vdemeester

Copy link
Contributor

Choose a reason for hiding this comment

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

I think custom task is not resource, its just concept. So may be this is fine to have

piyush-garg
piyush-garg reviewed Jan 16, 2024
View reviewed changes
pkg/test/controller.go Outdated
@@ -114,6 +118,13 @@ func seedTestData(t *testing.T, ctx context.Context, d Data) (Clients, Informers
t.Fatal(err)
}
}
c.Pipeline.PrependReactor("*", "customruns", test.AddToInformer(t, i.CustomRun.Informer().GetIndexer()))
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest not to use this and this is temporary and removed later, this is very specific for v1beta1 of resources which are deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your suggestion. Since this is my first PR, I referred the implementation of TaskRun. If it is not recommended to use c.Pipeline.PrependReactor, then which method should be used as a replacement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this temporary implementation will be replaced later, can we keep and wait for it to be replaced in subsequent PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not saying about c.Pipeline.PrependReactor. I am saying that this whole function seedTestData will be removed.

You have to use SeedTestData in tests instead of SeedV1beta1TestData and you will not need these changes in test pkg. This test controller is very specific for resources whose v1 is available and v1beta1 is deprecated like task, taskrun, pipeline and pipelinerun

@piyush-garg
Copy link
Contributor

I would request to add some e2e tests also for this

@stillfox-lee
Copy link
Contributor Author

I would request to add some e2e tests also for this

I will add e2e test later.

@piyush-garg
Copy link
Contributor

/test pull-tekton-cli-integration-tests

@piyush-garg
Copy link
Contributor

@stillfox-lee can you please handle the review comments, would like to get this PR for next release.

cc @vinamra28 if you can also review

@stillfox-lee
Copy link
Contributor Author

stillfox-lee commented Jan 24, 2024 •
edited

@stillfox-lee can you please handle the review comments, would like to get this PR for next release.

cc @vinamra28 if you can also review

Sorry for the delay. I have added e2e-test. Please have a look.

@piyush-garg
Copy link
Contributor

pkg/test/controller.go

Thanks for doing that. Changes looks good to me. Just 2 things

  1. Can you please remove the changes from this file pkg/test/controller.go These are not required
  2. Please squash all the commits into one.

@stillfox-lee
Copy link
Contributor Author

ping @piyush-garg @vinamra28

@piyush-garg
Copy link
Contributor

/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: piyush-garg

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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2024
@vdemeester
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2024
@tekton-robot tekton-robot merged commit c1705d8 into tektoncd:main Jan 25, 2024
8 checks passed
@stillfox-lee stillfox-lee deleted the customrun/list branch January 26, 2024 02:34
piyush-garg added a commit that referenced this pull request Feb 7, 2024
@piyush-garg
New version v0.35.0
c21271e 
#2212 | [vinamra28] Bump tkn to latest version v0.34.0 | 2024/01/12-11:26
#2215 | [dependabot[bot]] Bump github.com/sigstore/sigstore from 1.8.0 to 1.8.1 | 2024/01/18-06:22
#2218 | [dependabot[bot]] Bump k8s.io/cli-runtime from 0.26.12 to 0.26.13 | 2024/01/18-15:56
#2217 | [dependabot[bot]] Bump github.com/google/go-containerregistry from 0.17.0 to 0.18.0 | 2024/01/18-19:46
#2220 | [dependabot[bot]] Bump github.com/docker/cli | 2024/01/19-16:39
#2219 | [dependabot[bot]] Bump github.com/docker/docker | 2024/01/22-06:43
#2221 | [dependabot[bot]] Bump github.com/tektoncd/pipeline from 0.55.0 to 0.56.0 | 2024/01/22-07:23
#2224 | [dependabot[bot]] Bump github.com/docker/cli | 2024/01/25-05:11
#2225 | [dependabot[bot]] Bump github.com/docker/docker | 2024/01/25-07:37
#2213 | [lsy] feat: add CustomRun commad with list | 2024/01/25-13:25
null | [dependabot[bot]] Bump github.com/google/go-containerregistry from 0.18.0 to 0.19.0 | 2024/01/30-15:17
null | [Piyush Garg] Bump hashicorp/vault/api to v1.11.0 | 2024/01/31-06:47
null | [dependabot[bot]] Bump github.com/tektoncd/hub from 1.15.1 to 1.16.0 | 2024/02/01-15:36
null | [dependabot[bot]] Bump github.com/docker/cli | 2024/02/01-16:14
null | [dependabot[bot]] Bump github.com/docker/docker | 2024/02/01-17:52
null | [Piyush Garg] Bump tektoncd/chains to v0.20.0 | 2024/02/02-09:46
null | [Piyush Garg] Move chains cmd to v1 | 2024/02/02-09:46
null | [dependabot[bot]] Bump github.com/docker/cli | 2024/02/07-07:15
null | [dependabot[bot]] Bump github.com/docker/docker | 2024/02/07-08:01
null | [Piyush Garg] Bump triggers to v0.26.0 | 2024/02/07-08:39

Signed-off-by: Piyush Garg <pgarg@redhat.com>
@vinamra28 vinamra28 mentioned this pull request May 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@vinamra28 vinamra28 Awaiting requested review from vinamra28

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.

Support CustomRun list command.
4 participants
@stillfox-lee @tekton-robot @piyush-garg @vdemeester