-
Notifications
You must be signed in to change notification settings - Fork 208
Allow specifying SSH key for the Git repository where to fetch private Helm charts #2863
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.
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
Merged
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
6bcec75
Allow specifying SSH key for the Git repository where used to fetch p…
nghialv aac3d9d
Update pkg/config/piped.go
nghialv a9c370c
Fix error
nghialv 0ef87ed
Fix
nghialv b78b32d
Add type field to HelmChartRepository config
nghialv d2f46d9
Fix error
nghialv File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,6 +90,11 @@ func (s *PipedSpec) Validate() error { | |
| if s.SyncInterval < 0 { | ||
| return errors.New("syncInterval must be greater than or equal to 0") | ||
| } | ||
| for _, r := range s.ChartRepositories { | ||
| if err := r.Validate(); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| if s.SecretManagement != nil { | ||
| if err := s.SecretManagement.Validate(); err != nil { | ||
| return err | ||
|
|
@@ -256,6 +261,59 @@ type HelmChartRepository struct { | |
| Password string `json:"password"` | ||
| // Whether to skip TLS certificate checks for the repository or not. | ||
| Insecure bool `json:"insecure"` | ||
|
|
||
| // Remote address of the Git repository used to clone Helm charts. | ||
| // e.g. [email protected]:org/repo.git | ||
| GitRemote string `json:"gitRemote"` | ||
| // The path to the private ssh key file used while cloning Helm charts from above Git repository. | ||
| SSHKeyFile string `json:"sshKeyFile"` | ||
| } | ||
|
|
||
| func (r *HelmChartRepository) IsHTTPRepository() bool { | ||
| return r.Name != "" && r.Address != "" | ||
| } | ||
|
|
||
| func (r *HelmChartRepository) IsGitRepository() bool { | ||
| return r.GitRemote != "" | ||
| } | ||
|
|
||
| func (r *HelmChartRepository) Validate() error { | ||
| if r.IsHTTPRepository() { | ||
| if r.Name == "" { | ||
| return errors.New("name must be set") | ||
| } | ||
| if r.Address == "" { | ||
| return errors.New("address must be set") | ||
| } | ||
| } | ||
|
|
||
| if r.IsGitRepository() { | ||
| if r.GitRemote == "" { | ||
| return errors.New("gitRemote must be set") | ||
| } | ||
| } | ||
|
|
||
| return errors.New("either HTTP repository or Git repository must be configured") | ||
| } | ||
|
|
||
| func (s *PipedSpec) HTTPHelmChartRepositories() []HelmChartRepository { | ||
| repos := make([]HelmChartRepository, 0, len(s.ChartRepositories)) | ||
| for _, r := range s.ChartRepositories { | ||
| if r.IsHTTPRepository() { | ||
| repos = append(repos, r) | ||
| } | ||
| } | ||
| return repos | ||
| } | ||
|
|
||
| func (s *PipedSpec) GitHelmChartRepositories() []HelmChartRepository { | ||
| repos := make([]HelmChartRepository, 0, len(s.ChartRepositories)) | ||
| for _, r := range s.ChartRepositories { | ||
| if r.IsGitRepository() { | ||
| repos = append(repos, r) | ||
| } | ||
| } | ||
| return repos | ||
| } | ||
|
|
||
| type PipedCloudProvider struct { | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get your point. Fixed it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or should we add a
typefield, which should be eitherHTTPorGIT?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
typefield default toHTTP, and if users want to use typeGitthey 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 thetypefield.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Exactly as what you said. But the
GITtype 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
typefield. PTAL.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess the
typefield is a bit tedious as well, I also don't have any strong opinion though.I feel like just adding a comment can make it clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, we may add more types value to that field. In that case, adding type field and making type default to
HTTPis good for future addition 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright.