cli icon indicating copy to clipboard operation
cli copied to clipboard

Add --use-workspace-defaults

Open AeroNotix opened this issue 4 years ago • 33 comments

  • tkn task start always asks for the configuration of the workspaces, even if you have:
  default-task-run-workspace-binding: |
    emptyDir: {}

In the config-defaults.

If there is another way to achieve this, please let me know.

Add a new flag `--use-workspace-default` to use the configured default workspace for the task run

AeroNotix avatar Aug 29 '21 23:08 AeroNotix

CLA Signed

The committers are authorized under a signed CLA.

  • :white_check_mark: Aaron France (0de0504a6e5a7bbdb46db9e63c1458ff594ae96b)

Hi @AeroNotix. 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 avatar Aug 29 '21 23:08 tekton-robot

/assign @danielhelfand

AeroNotix avatar Aug 30 '21 01:08 AeroNotix

if you set UseParamDefaults as false, it should use the workspace defaults? If this doesn't happen like this it would be a bug isnt it ?

(also if we go forward on this please consider adding a test)

/ok-to-test

chmouel avatar Aug 30 '21 09:08 chmouel

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/task/start.go 80.9% 81.0% 0.1

tekton-robot avatar Aug 30 '21 09:08 tekton-robot

@chmouel no, using just --use-param-defaults=false without what I added, still prompts for the workspace configuration.

AeroNotix avatar Aug 30 '21 09:08 AeroNotix

/retest

AeroNotix avatar Nov 24 '21 17:11 AeroNotix

@danielhelfand can I get some help moving this along, please? What do I need to do/answer/change/discover/debug etc.

AeroNotix avatar Nov 24 '21 17:11 AeroNotix

The CI failure looks to be a documentantion related thing.. what do I need to fix?

AeroNotix avatar Nov 24 '21 17:11 AeroNotix

CLA Signed

The committers are authorized under a signed CLA.

  • :white_check_mark: Aaron France (0de0504a6e5a7bbdb46db9e63c1458ff594ae96b, be37447e32a089068c135db111d62dcda361c75b)

vinamra28 avatar Jan 28 '22 11:01 vinamra28

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/task/start.go 81.0% 81.1% 0.1

tekton-robot avatar Jan 28 '22 16:01 tekton-robot

@pradeepitm12 @vinamra28 @chmouel I think this we can close as #1465 is merged

piyush-garg avatar Feb 08 '22 10:02 piyush-garg

@pradeepitm12 @vinamra28 @chmouel I think this we can close as #1465 is merged

@piyush-garg I think we can still consider this as this is solving a different problem. There is a field in configmap as config-defaults where we can set the value of key default-task-run-workspace-binding. If I am not wrong then this PR is to handle this scenario :sweat_smile: #1465 is just to handle the binding of optional workspaces. This PR is to handle all the workspaces. It's similar to --use-params-default flag

vinamra28 avatar Feb 08 '22 10:02 vinamra28

@pradeepitm12 @vinamra28 @chmouel I think this we can close as #1465 is merged

@piyush-garg I think we can still consider this as this is solving a different problem. There is a field in configmap as config-defaults where we can set the value of key default-task-run-workspace-binding. If I am not wrong then this PR is to handle this scenario sweat_smile

Aah ok, this is for skipping all and #1465 is for skipping optional

piyush-garg avatar Feb 08 '22 11:02 piyush-garg

/retest

piyush-garg avatar Apr 18 '22 13:04 piyush-garg

@AeroNotix can you please rebase with upstream/main and try to repush ? Also could you please squash the commits?

vinamra28 avatar Apr 20 '22 07:04 vinamra28

You can squash the commits when you merge, by doing a squash commit. I'll rebase tho.

AeroNotix avatar Apr 20 '22 09:04 AeroNotix

You can squash the commits when you merge, by doing a squash commit. I'll rebase tho.

@AeroNotix there is a bot in place which performs the merging so won't be able to do that. Squashing of the commits needs to be done from your end 😅

vinamra28 avatar Apr 20 '22 09:04 vinamra28

Interesting so you use a bot to do some human's work but leave trivially automatable work to the humans. Quite an interesting workflow from a product which is all about automation :)

I'll squash ;) 👍🏻

AeroNotix avatar Apr 20 '22 09:04 AeroNotix

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/task/start.go 81.0% 81.1% 0.1

tekton-robot avatar Apr 20 '22 10:04 tekton-robot

@vinamra28 do I need to do something with that failure?

AeroNotix avatar Apr 20 '22 10:04 AeroNotix

@vinamra28 do I need to do something with that failure?

@AeroNotix could you please run make generated. It will generate some man pages that you need to commit and push. Also can you please do the same for ClusterTask and Pipeline as well?

vinamra28 avatar Apr 20 '22 10:04 vinamra28

Also can you please do the same for ClusterTask and Pipeline as well?

What do you mean? Add the same flag?

AeroNotix avatar Apr 20 '22 10:04 AeroNotix

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/task/start.go 81.0% 81.1% 0.1

tekton-robot avatar Apr 20 '22 10:04 tekton-robot

Added the same flags for cluster/pipeline.

AeroNotix avatar Apr 20 '22 11:04 AeroNotix

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/clustertask/start.go 80.2% 80.3% 0.1
pkg/cmd/pipeline/start.go 66.3% 66.4% 0.1
pkg/cmd/task/start.go 81.0% 81.1% 0.1

tekton-robot avatar Apr 20 '22 11:04 tekton-robot

/retest

vinamra28 avatar Apr 21 '22 06:04 vinamra28

@vinamra28 good to merge now?

AeroNotix avatar Apr 21 '22 14:04 AeroNotix

Added a release-note block, it should be good to merge now /cc @tektoncd/cli-maintainers

vdemeester avatar Apr 25 '22 07:04 vdemeester

Can we please add some tests for this?

piyush-garg avatar Apr 26 '22 11:04 piyush-garg