Allow specifying SSH key for the Git repository where to fetch private Helm charts #2863
Conversation
…rivate Helm charts
|
Code coverage for golang is
|
Co-authored-by: Khanh Tran <32532742+khanhtc1202@users.noreply.github.com>
pkg/config/piped.go
Outdated
| } | ||
| } | ||
|
|
||
| return errors.New("either HTTP repository or Git repository must be configured") |
There was a problem hiding this comment.
I think this is not right, since in case both or one of HTTP or GitRemote pattern are provided, the error will be returned too. We may need to add a return after L293 or change the implementation.
There was a problem hiding this comment.
Get your point. Fixed it!
There was a problem hiding this comment.
Or should we add a type field, which should be either HTTP or GIT?
There was a problem hiding this comment.
That's nice too 👍
Just want to confirm one point: do we only allow one of those types to be configured, not both at the same time for one chartRepo, right?
There was a problem hiding this comment.
In such case, the current implementation is LGTM, may be we don't need to add type to keep the configuration simple 😄
There was a problem hiding this comment.
In that case, the validation logic is easier for us but looks like it's not simple for our users. Since to keep support for running applications, we need to make the type field default to HTTP, and if users want to use type Git they have to explicitly specify that value in the configuration. But I don't have a strong opinion on this, just feel like it will be easier for me if I'm going to configure this without the type field.
There was a problem hiding this comment.
Yeah. Exactly as what you said. But the GIT type is only needed when user wants to specify a different SSH key for that repository, so I guess that won't cause much trouble for the user.
Btw, I added the type field. PTAL.
There was a problem hiding this comment.
Yeah, I guess the type field is a bit tedious as well, I also don't have any strong opinion though.
I see. Just feel weird when seeing this check.
I feel like just adding a comment can make it clear.
There was a problem hiding this comment.
Okay, we may add more types value to that field. In that case, adding type field and making type default to HTTP is good for future addition 👍
|
Code coverage for golang is
|
|
Code coverage for golang is
|
| } | ||
|
|
||
| func (r *HelmChartRepository) IsHTTPRepository() bool { | ||
| return r.Type == HTTPHelmChartRepository |
There was a problem hiding this comment.
| return r.Type == HTTPHelmChartRepository | |
| return strings.ToUpper(r.Type) == HTTPHelmChartRepository |
nits, users may insert HTTP, http or Http as well. Or we may need to check for the type field in the validation function at L293.
There was a problem hiding this comment.
I don't think that we should allow that. Like, KubernetesApp not K8sApp or kubernetesApp.
There was a problem hiding this comment.
I also think we should distinguish between lowercase and uppercase.
There was a problem hiding this comment.
You're right, in that case, we may need to add validate r.Type to the validate function at line L293, and the current error message (at L311) return Git instead of GIT
https://github.com/pipe-cd/pipe/pull/2863/files#diff-49258a851400c4bc54c40491b440d7edf1dd45eda52c9eccd3f5371af15e8409R311
There was a problem hiding this comment.
Yep, that should be:
return fmt.Errorf("either %s repository or %s repository must be configured", HTTPHelmChartRepository, GITHelmChartRepository)
|
Way to go 👍 |
|
Code coverage for golang is
|
|
Nice work |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
Basically, the same SSH key should be used for simplicity but users can specify another SSH key for the Git repositories where to fetch private Helm charts.
In that case, the path file of the private SSH key file must be specified in
chartRepositoriesfield of Piped configuration.Which issue(s) this PR fixes:
Fixes #2759
Does this PR introduce a user-facing change?: