Skip to content

Conversation

@pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Feb 14, 2022

Adapt the helm-template.sh script to use helmfile instead of helm directly. After #1805, the values files don't exist anymore, so this PR uses helmfile for rendering all helm charts using the actual templated values file.

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

@pcapriotti pcapriotti force-pushed the pcapriotti/helmfile-template branch from d20a43e to 84bee8d Compare February 14, 2022 15:32
Base automatically changed from pcapriotti/add-pull-policy to develop February 14, 2022 17:22
@pcapriotti pcapriotti force-pushed the pcapriotti/helmfile-template branch from 84bee8d to 96b08b7 Compare February 15, 2022 09:53
@pcapriotti pcapriotti mentioned this pull request Feb 15, 2022
6 tasks
@pcapriotti pcapriotti requested a review from jschaul February 15, 2022 13:23
Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

Doesn't work.

make helm-template-wire-server
(... snip for brevity...)
/home/user/Documents/git/wire-server/hack/bin/selfsigned-kubernetes.sh: line 29: FEDERATION_DOMAIN_BASE: you must provide a FEDERATION_DOMAIN_BASE env variable
make: *** [Makefile:527: helm-template-wire-server] Error 1

TOP_LEVEL="$DIR/../.."
CHARTS_DIR="${TOP_LEVEL}/.local/charts"
: "${FEDERATION_DOMAIN:=example.com}"
: "${NAMESPACE:=namespace1}"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with this syntax. Is that the same as

NAMESPACE=${NAMESPACE:-"namespace1"}

? i.e. set-to-default-if-not-exists? Or this hardcodes/overrides this variable to always be that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's the same as that. ${x:=foo} sets $x to foo if not set, and returns the value of $x. The colon is just an empty statement. It's a way to evaluate an expression (usually with side effects), without executing any command.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for the explanations!


set -e

chart=${1:?$USAGE}
Copy link
Member

Choose a reason for hiding this comment

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

what happened to this variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how to render a single chart with helmfile.

@jschaul
Copy link
Member

jschaul commented Feb 15, 2022

I think it's useful to be able to template a single chart, rather than "everything". How is this script intended to be used? Some docs would be nice for usage and purpose.

@pcapriotti
Copy link
Contributor Author

Sorry, I wasn't aware of the Makefile target. I guess I'll remove the argument, since the current script doesn't take any, and just renders all charts.

I think it's useful to be able to template a single chart, rather than "everything". How is this script intended to be used? Some docs would be nice for usage and purpose.

I've been using it by piping the output of the script to jq (via some yaml -> json conversion script). Or one could use yq directly. This works well enough for me. Not sure what docs are needed, the script just works by itself without any arguments.

@pcapriotti pcapriotti requested a review from jschaul February 15, 2022 16:16
@jschaul
Copy link
Member

jschaul commented Feb 16, 2022

@pcapriotti This PR still does not work (as written in my previous comment) after the latest changes, with or without the makefile target:

./hack/bin/helm-template.sh
/home/user/Documents/git/wire-server/hack/bin/selfsigned-kubernetes.sh: line 29: FEDERATION_DOMAIN_BASE: you must provide a FEDERATION_DOMAIN_BASE env variable

So I cannot approve this yet, please set the necessary environment variable in the script matching the domain.

@pcapriotti
Copy link
Contributor Author

@pcapriotti This PR still does not work (as written in my previous comment) after the latest changes, with or without the makefile target:

./hack/bin/helm-template.sh
/home/user/Documents/git/wire-server/hack/bin/selfsigned-kubernetes.sh: line 29: FEDERATION_DOMAIN_BASE: you must provide a FEDERATION_DOMAIN_BASE env variable

So I cannot approve this yet, please set the necessary environment variable in the script matching the domain.

You're right, I hadn't tested this correctly. Should be fixed now.

@pcapriotti
Copy link
Contributor Author

I'm not sure how to make this usable for everyone, so I'll just move into my own collection of scripts and close the PR. Feel free to reopen if you want to brig this back.

@pcapriotti pcapriotti closed this Mar 25, 2022
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