-
Notifications
You must be signed in to change notification settings - Fork 105
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
Helm Chart for HNC installation #354
Helm Chart for HNC installation #354
Conversation
/cc @adrianludwin |
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.
So sorry, I started a review and got side tracked. Here are a few inital thoughts, I'll find some time soon to give it a closer look. I'm traveling the next few weeks so apologize in advanced.
hack/helm_patches/update-helm.sh
Outdated
# limitations under the License. | ||
|
||
CURDIR=$(pwd) | ||
YQ=${CURDIR}/bin/yq |
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.
Copy Paste from makefile: I wouldn't expect yq
to live in the CURDIR
. Maybe add some logic like this instead?
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.
@rjbez17
I may not have understood your comment sufficiently...
update-helm.sh
wil be called from Makefile via make charts
and yq
will be installed in ${CURDIR}/bin
before execution update-helm.sh
.
Therefore, isn't it expected that yq
exists in ${CURDIR}/bin
?
hierarchical-namespaces/Makefile
Lines 501 to 505 in b2ccf30
.PHONY: yq | |
yq: ## Download yq locally if necessary. | |
@GOBIN=${CURDIR}/bin GO111MODULE=on go install github.com/mikefarah/yq/v4@v4.34.1 | |
charts: yq manifests |
hack/helm_patches/update-helm.sh
Outdated
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
CURDIR=$(pwd) |
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 is a much safer way to achieve this (ROOTDIR
or REPODIR
is more descriptive I think):
ROOTDIR="$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )/../"
Makefile
Outdated
|
||
charts: yq manifests | ||
# Generate CRDs template from manifests/crds.yaml | ||
@rm -rf charts/hnc/crds |
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.
If you change this to @rm -rf charts/hnc/crds/*
you can avoid the mkdir
command following it
Makefile
Outdated
cat ${MANIFESTDIR}/crds.yaml | ${YQ} -N -s '.metadata.name + ".yaml"' | ||
|
||
# Generate helm templates from manifests | ||
@rm -rf charts/hnc/templates |
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.
Same here
/ok-to-test |
b2ccf30
to
676bbdb
Compare
8c1ae69
to
cc1ae7e
Compare
Thanks @rjbez17 ! |
charts/hnc/values.yaml
Outdated
@@ -0,0 +1,42 @@ | |||
# Caution: Don't change namespaceOverride from hnc-system. |
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.
Is there a reason we don't just hardcode these in _helpers.tpl
since the aren't supposed to be overriden?
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.
@rjbez17
As you say, I think it good to hardcode them.
So I've fixed that.
cc1ae7e
to
324a67a
Compare
Sorry for the delay here. Finally found some time to pull the branch and play with the templating. The ONLY thing left that I can find is can you please make:
Building the manifests (
|
@rjbez17 Of course, it's ok to set |
Yeah, I was on the fence with it, but it seems that current manifest generation turns it on by default so I was thinking we'd do the same. We can keep at as |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mochizuki875, rjbez17 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@rjbez17 Thanks for your reviews and approve! |
Thanks guys, I'm really looking forward to see this helm chart as artefact hosted on github pages! |
What type of PR is this?:
/kind feature
What this PR does / why we need it:
Provide HNC installation method via Helm.
Previously, it was disccussed in #46 and #62.
However, the problem that how to keep the chart up-to-date with changes in the config directory has not been resolved.
In this PR, I've added a mechanism to auto generate chart from manifest file generated from config directory using kustomize.
How to use:
Clone this repository.
$ git clone https://github.com/mochizuki875/hierarchical-namespaces.git -b feature_hnc_helm
Set environment variables.
They will be actually set in release process.
Generate chart.
$ make charts
Install chart.
You can do any setting in
charts/hnc/values.yaml
before installation.eg: setting hncExcludeNamespaces, enable HRQ, etc.
Tested:
E2E tests has passed.
Related:
#46
#62
Special notes for your reviewer:
Packaging and publishing to chart repository are not included.
In many cases, charts are placed in gh-pages branch and published as Helm repository using GitHub Pages.
eg:
https://github.com/prometheus-community/helm-charts/tree/gh-pages
Onece gh-pages branch is created add published as GitHub Pages, I think we can include these process in
cloudbuild.yaml
.