Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Add flag to specify alternative default git ref #83

Merged
merged 1 commit into from
Oct 29, 2019

Conversation

nweedo3
Copy link
Contributor

@nweedo3 nweedo3 commented Oct 22, 2019

Add the --git-default-chart-ref flag which enables the modification
of the ref used to clone a chart if the .spec.git.ref field is
unspecified. The original default of master is maintained.

Related issue: #80

@@ -145,6 +145,7 @@ spec:
{{- end }}
- --git-timeout={{ .Values.git.timeout }}
- --git-poll-interval={{ .Values.git.pollInterval }}
- --git-default-chart-ref={{ .Values.git.defaultChartRef }}
Copy link
Member

Choose a reason for hiding this comment

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

Wrap this in an if to maintain backwards compatibility:

{{- if .Values.git.defaultChartRef }}
- --git-default-chart-ref={{ .Values.git.defaultChartRef }}
{{- end }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stefanprodan sure. Would you prefer the entire PR in one commit, or are you fine with me adding the review changes on top of the original commit?

Copy link
Member

Choose a reason for hiding this comment

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

One commit is fine, amend your commit and force push it. Thanks

@@ -5,7 +5,7 @@ import (
"strings"

"github.com/ghodss/yaml"
"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this alias, it has no effect since it's the same with the package name.

@nweedo3
Copy link
Contributor Author

nweedo3 commented Oct 22, 2019

@stefanprodan the changes should be good to go now - thanks for the review.

@nweedo3
Copy link
Contributor Author

nweedo3 commented Oct 29, 2019

@stefanprodan do you have any further updates on this? Thanks.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -112,6 +113,7 @@ func init() {

gitTimeout = fs.Duration("git-timeout", 20*time.Second, "duration after which git operations time out")
gitPollInterval = fs.Duration("git-poll-interval", 5*time.Minute, "period on which to poll git chart sources for changes")
gitDefaultChartRef = fs.String("git-default-chart-ref", "master", "ref to clone chart from if ref is unspecified in a HelmRelease")
Copy link
Member

Choose a reason for hiding this comment

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

I think the length of the flag can be reduced to --git-default-ref, as the only Git thing the operator does is syncing charts.

Add the `--git-default-ref` flag which enables the modification
of the ref used to clone a chart if the `.spec.git.ref` field is
unspecified. The original default of `master` is maintained.

Related issue: fluxcd#80
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Thank you @niall-weedon 🥇

@hiddeco hiddeco merged commit c6feaa0 into fluxcd:master Oct 29, 2019
@nweedo3
Copy link
Contributor Author

nweedo3 commented Oct 29, 2019

No problem @hiddeco, thanks for the review! :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants