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

Initial helm chart #330

Merged
merged 5 commits into from
Mar 16, 2021
Merged

Conversation

mkilchhofer
Copy link
Contributor

@mkilchhofer mkilchhofer commented Feb 2, 2021

Deploy via helm chart

I use nebraska in my homelab to update my flatcar hosts esp. to reduce the traffic for downloading the new images.
For this I made a helm chart.

Are you interested in this? I found not that much information about deploying and hosting nebraska in production.

What also could be nice to provide:

  • docs about deploying in Kubernetes (with example yamls)
  • docs about deploying in Kubernetes with Kustomize
  • GitHub Actions for hosting the chart as a static resource in gh-pages (done with Helm Chart Releaser GitHub Action)

How to use

  1. clone the repo
  2. Update dependencies
 helm dependency update ./charts/nebraska
  1. then run the following helm command
    helm install my-nebraska ./charts/nebraska -n kube-system
  2. (customize if needed)
    helm upgrade my-nebraska ./charts/nebraska --set ingress.hosts={my-nebraska.example.com}

Testing done

@joaquimrocha
Copy link
Collaborator

@johananl , Do you wanna take a look at this one?

Copy link
Contributor

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

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

@mkilchhofer thank you for opening the PR. Really great work. Some NITs, can you please address them?

deploy/helm/templates/secrets.yaml Outdated Show resolved Hide resolved
deploy/helm/templates/deployment.yaml Outdated Show resolved Hide resolved
deploy/helm/Chart.yaml Outdated Show resolved Hide resolved
deploy/helm/Chart.yaml Outdated Show resolved Hide resolved
deploy/helm/Chart.yaml Outdated Show resolved Hide resolved
@mkilchhofer mkilchhofer force-pushed the feature/helm-chart branch 4 times, most recently from 734b10b to 6b5310f Compare February 28, 2021 14:28
@mkilchhofer
Copy link
Contributor Author

Hi @knrt10,
Do you plan to use the branch gh-pages and GitHub Pages? If not, I can also add a github action workflow which releases the chart through a GitHub release and updating an index.yaml file inside the branch gh-pages.

@joaquimrocha
Copy link
Collaborator

Hi @mkilchhofer , we have our own documentation system, so we don't plan on using gh-pages (unless I misunderstood your point).
A Github action to update stuff on release sounds fine. I intend to do that for the container image as well in the future.

@joaquimrocha
Copy link
Collaborator

the docs, BTW: https://kinvolk.io/docs/nebraska/latest

@mkilchhofer
Copy link
Contributor Author

mkilchhofer commented Mar 1, 2021

Hi @mkilchhofer , we have our own documentation system, so we don't plan on using gh-pages (unless I misunderstood your point).
A Github action to update stuff on release sounds fine. I intend to do that for the container image as well in the future.

Okay, perfect 👌
We need an empty gh-pages branch for this, which I cannot provision inside this PR:

$ # Prepare an empty branch
$ git checkout --orphan gh-pages
$ git reset
$ rm -r *

$ # Generate the index structure
$ cat <<EOF > index.yaml
apiVersion: v1
entries: {}
generated: "2021-03-01T00:00:00Z"
EOF

$ # Add the file
$ git add index.yaml
$ git commit -m 'Add empty index.yaml'
$ git push

@mkilchhofer mkilchhofer force-pushed the feature/helm-chart branch 2 times, most recently from 1d8555e to 1469a3a Compare March 1, 2021 19:39
@mkilchhofer mkilchhofer changed the title Draft: Initial helm chart Initial helm chart Mar 1, 2021
@mkilchhofer mkilchhofer marked this pull request as ready for review March 1, 2021 21:52
@mkilchhofer mkilchhofer requested a review from knrt10 March 1, 2021 21:52
@mkilchhofer mkilchhofer force-pushed the feature/helm-chart branch 2 times, most recently from 388b8dc to b6f81ae Compare March 3, 2021 13:30
@joaquimrocha
Copy link
Collaborator

Thanks again for another big push @mkilchhofer !
I was going to take a look now too but I'd like to ask you to squash the commits that you think should go together.
Ideally we should have atomic commits. Depending on this case, it may mean that all your commits should be squashed, or maybe there are a few that should be added separately.
However, we usually never squash commits together (unless their small fixups to one base commit) when merging PRs. So I kindly ask you to squash the commits to create the atomic ones you see as adequate, and that'll make the PR easier for another review round and testing.

@mkilchhofer
Copy link
Contributor Author

mkilchhofer commented Mar 5, 2021

Done :)
Please remember to bootstrap the new branch before merging.

Oh and please have a look at the releases page of the source branch repo: https://github.com/mkilchhofer/nebraska/releases
and also the repo tags: https://github.com/mkilchhofer/nebraska/tags.
The releaser action by default creates a GitHub Release for helm chart updates with this name convention: {{ .Name }}-{{ .Version }} (.Name = chart name, .Version = version of the Chart). This results in releases with the name "nebraska-x.y.z". Maybe we want to prefix the chart releases or simply replace the chart name with "helm" or something like that (as there is only one chart in this repo)? If so, we can configure the releaser like this: cr.yaml (juicefs-csi-driver)

To not confuse people with application releases and helm releases, I set the Release Name Template to {{ .Name }}-helm-{{ .Version }}. I hope, this is ok for you?

@mkilchhofer mkilchhofer force-pushed the feature/helm-chart branch 2 times, most recently from 5345eff to ca5f9a2 Compare March 5, 2021 15:38
Copy link
Contributor

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

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

Looks great @mkilchhofer. Some last Nits. Also I tested it, it works fine. Made some changes to description for user as a first time install. Hope that's okay.

charts/nebraska/README.md Show resolved Hide resolved
charts/nebraska/README.md Outdated Show resolved Hide resolved
This is required for clusters with a runAsNonRoot policy for regular workload
(eg. Lokomotive).
Copy link
Contributor

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

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

Thanks for the awesome work @mkilchhofer. LGTM. Tested it again with the new changes. Works fine.

@joaquimrocha joaquimrocha merged commit f6943df into flatcar:master Mar 16, 2021
@joaquimrocha
Copy link
Collaborator

Hi @knrt10,
Do you plan to use the branch gh-pages and GitHub Pages? If not, I can also add a github action workflow which releases the chart through a GitHub release and updating an index.yaml file inside the branch gh-pages.

Hi @mkilchhofer , I merged the PR but didn't realize it was still trying to use the gh-pages branch (which we don't use).
Can we use an alternative for this? Maybe using the Github Releases as you hinted in the message above, or does that still require the gh-pages branch?

Thanks.

@mkilchhofer
Copy link
Contributor Author

Hi @knrt10,
Do you plan to use the branch gh-pages and GitHub Pages? If not, I can also add a github action workflow which releases the chart through a GitHub release and updating an index.yaml file inside the branch gh-pages.

Hi @mkilchhofer , I merged the PR but didn't realize it was still trying to use the gh-pages branch (which we don't use).
Can we use an alternative for this? Maybe using the Github Releases as you hinted in the message above, or does that still require the gh-pages branch?

Thanks.

Hi @joaquimrocha, Maybe I didn't understand your statement correctly. What I understand was that you do not use the branch gh-pages so it is available for this intent :-)

@joaquimrocha
Copy link
Collaborator

No :)
I meant I'd prefer to keep not using that branch, but I am guessing there's no greater alternative if we don't intend to host it somewhere else?

@mkilchhofer
Copy link
Contributor Author

mkilchhofer commented Mar 16, 2021

Exactly. We need to serve an index.yaml via https to the helm users. A next step could be to list this chart on https://artifacthub.io/ (only an index pointing to an external repository) then we would need an additional file for this besides the index.yaml

mkilchhofer added a commit to mkilchhofer/k8s-gitops-services that referenced this pull request Mar 17, 2021
Contribution was accepted yesterday, see:
flatcar/nebraska#330
@mkilchhofer mkilchhofer deleted the feature/helm-chart branch April 8, 2021 19:40
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