-
Notifications
You must be signed in to change notification settings - Fork 96
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
Move config map to go #1268
Move config map to go #1268
Conversation
45f71be
to
e795535
Compare
295c011
to
5704515
Compare
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.
Huge PR and great work! Could we possibly split up some distinct pieces of this and merge it in one at a time? Extracting out all the gotohelm
changes would be a great start and then we can make sure they get appropriate tests as well.
I'd like to talk about the proposed schema changes in a bit more depth as well. I think that would be a good candidate for being extracted into it's own commit/PR.
b2f27bc
to
b6bfb03
Compare
8767ad7
to
7cf3055
Compare
pkg/helm/helm.go
Outdated
@@ -300,12 +304,18 @@ func (c *Client) Template(ctx context.Context, chart string, opts TemplateOption | |||
// to set it ourselves. | |||
client.ReleaseName = releaseName | |||
|
|||
previous := os.Getenv("HELM_CONFIG_HOME") | |||
os.Setenv("HELM_CONFIG_HOME", path.Join(c.configHome, "helm-config")) |
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.
Do we need to set this? I don't see anything reading this value
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'm not sure why it isn't picked up by the install action. I agree it might affect other parallel runs. Using other helm commands this env var is passed into helm cli subproces
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 was thinking that locate chart would work with
chart, err = client.ChartPathOptions.LocateChart(chart, &cli.EnvSettings{
RegistryConfig: envOr("HELM_REGISTRY_CONFIG", helmpath.ConfigPath("registry/config.json")),
RepositoryConfig: envOr("HELM_REPOSITORY_CONFIG", helmpath.ConfigPath("repositories.yaml")),
RepositoryCache: envOr("HELM_REPOSITORY_CACHE", helmpath.CachePath("repository")),
Debug: true,
})
apparently the c.configHome
needs to be injected into tree of those environment variables.
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.
Found the problem. Chart repository is relaying on env var. Here is the stack trace:
https://github.com/helm/helm/blob/15f76cf83c670a329b62c2b5ddeb0864ec99daec/pkg/repo/chartrepo.go#L78
https://github.com/helm/helm/blob/15f76cf83c670a329b62c2b5ddeb0864ec99daec/pkg/helmpath/home.go#L24
https://github.com/helm/helm/blob/15f76cf83c670a329b62c2b5ddeb0864ec99daec/pkg/helmpath/lazypath.go#L60
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.
Tests are passing, let's merge and refactor from there!
54894a0
to
079fd76
Compare
61579ed
to
933db92
Compare
Methods improves readability of Redpanda configuration.
Previous implementation would panic as type assertion was hardcoded for Ident.
When Prometheus Redpanda helm chart dependency is enabled, then DecodeYAMLFrom is not working due to missing ServiceMonitor custom resource in scheme. ``` === CONT TestConfigMap/14-prometheus-no-tls-values.yaml chart_test.go:182: Error Trace: /Users/rafalkorepta/workspace/redpanda/helm-charts/charts/redpanda/chart_test.go:182 Error: Received unexpected error: no kind "ServiceMonitor" is registered for version "monitoring.coreos.com/v1" in scheme "pkg/runtime/scheme.go:100" Test: TestConfigMap/14-prometheus-no-tls-values.yaml --- FAIL: TestConfigMap/14-prometheus-no-tls-values.yaml (0.08s) ```
Changes to the input values are anti-pattern and relay on order of execution. Our test shows that overprovioned are incorrectly set when CPU is bellowed `1000m`. Reference (overproviosned is set to false)[https://github.com/redpanda-data/helm-charts/blob/5e30ecbaf15da44691402f6128194d6639652845/charts/redpanda/testdata/ci/22-gke-tiered-storage-with-creds-values.yaml.tpl.golden#L481] https://github.com/redpanda-data/helm-charts/blob/5e30ecbaf15da44691402f6128194d6639652845/charts/redpanda/ci/22-gke-tiered-storage-with-creds-values.yaml.tpl#L33 https://github.com/redpanda-data/helm-charts/blob/5e30ecbaf15da44691402f6128194d6639652845/charts/redpanda/templates/_helpers.tpl#L167-L169 https://github.com/redpanda-data/helm-charts/blob/5e30ecbaf15da44691402f6128194d6639652845/charts/redpanda/templates/_helpers.tpl#L174
933db92
to
9ef78e3
Compare
|
||
address := fmt.Sprintf("%s-%d", Fullname(dot), int(i)) | ||
if ptr.Deref(values.External.Domain, "") != "" { | ||
// TODO: TPL DOES NOT WORK. Maybe tpl call could be removed |
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.
nit: tpl is now implemented
client, err := helm.New(helm.Options{ConfigHome: testutil.TempDir(t)}) | ||
require.NoError(t, err) | ||
|
||
// Downloading Redpanda helm chart release is required as client.Template |
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.
This feels fixable but we can worry about it later 👍
ec39960 Add ability to annotate functions with
ignore
statement99bb6e0 Fix typo from
interation
toiteration
b016311 Improve rewrite error reporting
6012dbb Add
RegexReplaceAll
sprig function75f28bb Generate values to helm template
Methods improves readability of Redpanda configuration.
IsAccessKeyReferenceValid
andIsSecretKeyReferenceValid
functions are not used.e0fad8c Fix
for
loops using selector expressionPrevious implementation would panic as type assertion was hardcoded for
ast.Ident
.2e38275 Change
tiered
helper function toStorage
methods6df252f Add Monitoring coreos.com custom resources to the scheme
When Prometheus Redpanda helm chart dependency is enabled, then DecodeYAMLFrom
is not working due to missing ServiceMonitor custom resource in scheme.
28d2f13 Convert config maps to go base implementation
97a80f2 Remove old config map implementation
Update Statefulset config map hash calculation
9ef78e3 Handle overprovisioned differently from the desire implementation
Changes to the input values are anti-pattern and relay on order of execution.
Our test shows that overprovioned are incorrectly set when CPU is bellowed
1000m
.Reference
(overproviosned is set to false)[https://github.com/redpanda-data/helm-charts/blob/5e30ecbaf15da44691402f6128194d6639652845/charts/redpanda/testdata/ci/22-gke-tiered-storage-with-creds-values.yaml.tpl.golden#L481]
helm-charts/charts/redpanda/ci/22-gke-tiered-storage-with-creds-values.yaml.tpl
Line 33 in 5e30ecb
helm-charts/charts/redpanda/templates/_helpers.tpl
Lines 167 to 169 in 5e30ecb
helm-charts/charts/redpanda/templates/_helpers.tpl
Line 174 in 5e30ecb
Follow up PRs
#1358
#1359