Add support for multiple git resolver configurations #8263
Conversation
tekton-robot
added
release-note
piyush-garg
force-pushed
the
add_multi_git_res_auth_ref
branch
2 times, most recently
from
52b64a2
to
16592ca
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-integration-tests |
|
/test pull-tekton-pipeline-go-coverage-df |
|
@piyush-garg: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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. |
|
/test pull-tekton-pipeline-alpha-integration-tests |
|
/test pull-tekton-pipeline-go-coverage-df |
|
@piyush-garg: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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. |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-integration-tests |
|
/test pull-tekton-pipeline-go-coverage-df |
|
@piyush-garg: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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. |
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
approved
There was a problem hiding this comment.
CodeQL is not happy 😅.
So I am ok to get this merge but it doesn't feel like something that scale that much. It will work fine for small teams, but I do feel like bigger team that would require tens or hundreds of identifier would instead use the parameter. Maybe it's just a documentation issue.
/cc @afrittoli
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chitrangpatel, vdemeester 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 |
piyush-garg
force-pushed
the
add_multi_git_res_auth_ref
branch
from
16592ca
to
dc348a1
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/retest |
There was a problem hiding this comment.
Thanks for this PR, it's a great functionality to add.
I left a new comments, the most important are about handling error cases and adding some code coverage to config.go; plus I would really like to see the redundant identifier go away from the configuration.
I think changing the GitResolverConfig from a struct to a map would simplify the code in a few places.
I know that comments about names sound picky, so feel free to ignore if you prefer. I think it would help with code readability though as I struggled a bit to associate variables with the value they carried as I could not associated the name to the value easily.
There are a few functions that take the param map as input now, only to extract the parameter that identifies the config name, so maybe they could take a string only. On the plus side, passing the params is more future-proof, so it's probably fine to keep them as they are.
|
/hold |
tekton-robot
added
the
do-not-merge/hold
|
Sorry to hold the PR, but since the structure of the configuration and name of the parameters are user facing, we should fix them before this is merge. @vdemeester @chitrangpatel WDYT |
No worries regarding this. Lets all approve and agree with user facing things, then we can proceed with merge |
piyush-garg
force-pushed
the
add_multi_git_res_auth_ref
branch
from
dc348a1
to
6c85bff
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
piyush-garg
force-pushed
the
add_multi_git_res_auth_ref
branch
from
6c85bff
to
d30340e
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
Hey @afrittoli @chitrangpatel @vdemeester I have done all the changes based on comments, please take a look now. Thanks |
docs/git-resolver.md
Outdated
| @@ -195,6 +191,114 @@ spec: | |||
| value: Ranni | |||
| ``` | |||
|
|
|||
| ### Specifying Configuration for Multiple Git Providers | |||
|
|
|||
| You can specify configurations for multiple providers and even multiple configurations for same provider to use in | |||
There was a problem hiding this comment.
NIT: I would recommend using impersonal sentences, i.e. instead of "You can specify...", "It is possible to specify configurations..." etc
This will add support for providing multiple configurations in git resolver configmap Configurations can be provided using unique key and that key can be passed as a param in the taskrun/pipelinerun to resolver Old format is supported to provide backward compatibility and docs added in details about how to use the feature. Unit and e2e test added Fixes tektoncd#5487
piyush-garg
force-pushed
the
add_multi_git_res_auth_ref
branch
from
d30340e
to
33d1398
Compare
@afrittoli I have addressed all the comments |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
|
||
| ### Example Configmap | ||
|
|
||
| You can add multiple configuration to `git-resolver-configmap` like this. All keys mentioned above are supported. |
| It is possible to specify configurations for multiple providers and even multiple configurations for same provider to use in | ||
| different tekton resources. You need to first add details in configmap with your unique identifier key prefix. | ||
| To use them in tekton resources, pass the unique key mentioned in configmap as an extra param to resolver with key | ||
| `configKey` and value your unique key. If no `configKey` param is passed, `default` will be used. You can specify |
|
/hold cancel |
tekton-robot
removed
the
do-not-merge/hold
This will add support for providing multiple configurations
in git resolver configmap
Configurations can be provided using unique key and that key
can be passed as a param in the taskrun/pipelinerun to resolver
Old format is supported to provide backward compatibility and
docs added in details about how to use the feature.
Unit and e2e test added
Fixes #5487
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes