Skip to content
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

Fix chart installation #401

Merged
merged 8 commits into from
Nov 7, 2024
Merged

Fix chart installation #401

merged 8 commits into from
Nov 7, 2024

Conversation

dwertent
Copy link
Contributor

@dwertent dwertent commented Nov 6, 2024

  • Adjust template placeholder decoding.
  • Enable replacement of the paladin image in the values file.
  • Remove cert-manager from the chart dependencies.

Note: run gradle clean -PdeleteCluster=true after pulling the changes

Signed-off-by: dwertent <[email protected]>
@@ -914,7 +915,7 @@ issuerRef:
if tlsConf.CertSpecTemplate != "" {
specTemplate = tlsConf.CertSpecTemplate
}
template, err := template.New("").Option("missingkey=error").Funcs(sprig.FuncMap()).Parse(specTemplate)
template, err := template.New("").Option("missingkey=error").Funcs(sprig.FuncMap()).Parse(adjustTemplatePlaceholders(specTemplate))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry @dwertent, I feel a little lost to understand why there's a codechange in the Operator (rather than the generation of the CRs). Going to see if I can understand by building the Helm charts to see what's inside of them.

I assert that from the Operator's perspective, the YAML should be valid. It should contain a valid Go template (or if we don't like go templates, we use a different Template engine like yq etc.).

The the thing generating the YAML needs to have templating itself, like Helm, then it's responsible for doing the right escaping in the Helm chart to make the result of the helm chart valid YAML-containing-a-go-tempalte.

Copy link
Contributor

Choose a reason for hiding this comment

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

I generated the helm chart, and the values I thought would be problematic look fine to me

"implementation":        '{{`{{ index .status.resolvedContractAddresses "zeto-impl-anon" }}`}}',

^^^ looks like should generate the right CR per Helm escaping rules... going to try and see if I can recreate a problem with this

Copy link
Contributor

@peterbroadhurst peterbroadhurst Nov 7, 2024

Choose a reason for hiding this comment

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

Ok - I think I understood what was being attempted here, and I don't think it was quite right to do it in the operator and handle bad templates generated by the helm charts.

We have two types of things:

  • YAML samples
  • Helm templates

We want to take our YAML samples, and turn them into HELM templates.

We do that in:

make prepare-operator-chart

So I have fixed that Makefile thing to escape the {{ characters.
Given it was Makefile code, the thing I had to hand to do this was sed.

We could move this to Gradle, and then we could use nicer tools. But I didn't have time today.

paladin/operator/Makefile

Lines 203 to 211 in fb4fa83

prepare-operator-chart: ## Copy samples to the Helm chart.
mkdir -p charts/paladin-operator/templates/samples
cp config/samples/core_v1* charts/paladin-operator/templates/samples/
cp config/samples/cert* charts/paladin-operator/templates/samples/
@for file in $(shell ls charts/paladin-operator/templates/samples/*.yaml ) ; do \
mv "$${file}" "$${file}.tmp"; \
sed -r 's/{{(.*)}}/{{ `{{\1}}` }}/g' "$${file}.tmp" > "$${file}"; \
rm "$${file}.tmp"; \
done

@dwertent dwertent merged commit 912bc51 into main Nov 7, 2024
4 checks passed
@dwertent dwertent deleted the temaplte-placehodlers branch November 7, 2024 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants