feat: add helm package to release assets when changes occur by callum-tait-pbx · Pull Request #220 · actions/actions-runner-controller

callum-tait-pbx · 2020-12-02T10:51:47Z

Creating a draft PR to get initial comments. @mumoshu

Problem
Currently, the Helm chart in the repo was manually created and is not part of the formal release. The static nature means changing templates 2 places and it also creates issues for me around automation as I don't want to have to monitor the repository for chart changes, nor do I want to come up with an complicated sync job to work around this. Attaching the built chart to the release would allow for a much simplier sync job for my internal feed.

Solution
Have the Helm chart produced as part of the CI and attached to the release so it is much easier to apply automation to as an external user of the solution. The need for this was highlighted in #195

Helm linting is on on the test workflow and helm charts are included in the publish if they are found to have changed beteen the previous tag and the latest tag

Limitations

The work assumes that all tags get published as a release as it gets the latest tag at the time (which will by the end of the workflow be the previous tag) and compares the changes between that tag and HEAD to see if any files were changed within the charts/ and config/crd/bases folder.
The helm package is simply attached to the release. The more standard approach for releasing charts in github actions https://github.com/helm/charts-repo-actions-demo/blob/master/.github/workflows/release.yaml would require a lot more work as it does not support appending to an already existing release and automatically creates the release.
The chart should only be built when there are changes to the charts folder itself as well as the CRDs. Ideally the entire chart should be defined once (including templates etc) however the rest of the solution doesn't change much so as an initial PR this limitation is OK and doesn't make it worse.

Notes
https://medium.com/@mattiaperi/create-a-public-helm-chart-repository-with-github-pages-49b180dbb417
https://jamiemagee.co.uk/blog/how-to-host-your-helm-chart-repository-on-github/

There are some interesting posts on how you can use GitHubs pages feature to use the repository itself as a Helm chart feed. To do this the pipeline would need to, in addition to the changes in this PR, update an index.yaml file and commit that into the history. This file would be stored under Docs/ and github pages would be configured to serve the Docs/ folder, any thoughts on this would be welcomed.

I have learnt I hate Makefiles

Helm package would fail as the app version wasn't semantic. Stripping v from version for Helm package step

callum-tait-pbx · 2020-12-10T19:54:24Z

I actually didn't mean to do those last 2 commits in this PR, I had the wrong VS code open 🤦 . I'll keep them seen as they are commited now. Would love a review when you get 10 @mumoshu

mumoshu · 2020-12-14T00:08:42Z

Makefile


 # Run tests
-test: generate fmt vet manifests
+test: create-release-dir generate fmt vet manifests helm-lint


Suggested change

test: create-release-dir generate fmt vet manifests helm-lint

test: generate fmt vet manifests helm-lint

I think we don't need create-release-dir dependency here.

If it's actually needed, I'd rather suggest move it to where it is depended. Like if manifests depends on create-release-dir , add create-release-dir as manifests's dependency.

mumoshu · 2020-12-14T00:10:54Z

Makefile


 # Upload release file to GitHub.
-github-release: release
+github-release: create-release-dir release


Suggested change

github-release: create-release-dir release

github-release: release

release seems to depend on create-release-dir so I think we don't need to re-add the dependency here

mumoshu · 2020-12-14T00:23:41Z

charts/actions-runner-controller/values.yaml

+# Only 1 authentication method can be enabled at a time
+# Values need to be base64'ed in values.yaml
+githubAuthenticationSchema:
+  app:
+    enabled: false
+    github_app_id: ""
+    github_app_installation_id: ""
+    github_app_private_key: ""
+  token:
+    enabled: false
+    github_token: ""


Suggested change

# Only 1 authentication method can be enabled at a time

# Values need to be base64'ed in values.yaml

githubAuthenticationSchema:

app:

enabled: false

github_app_id: ""

github_app_installation_id: ""

github_app_private_key: ""

token:

enabled: false

github_token: ""

# Set `enabled: true` and either specify github_app_* or github_token to let the chart

# create the K8s secret required by the controller to authenticate against GitHub API.

# All the values must NOT be base64-encoded, as the chart template base64-encodes values for you.

authSecret:

enabled: false

github_app_id: ""

github_app_installation_id: ""

github_app_private_key: ""

github_token: ""

I slightly prefer modeling this around the fact that we just create a single K8s secret containing various credentials.

That would allow us to easily enhance this secret/chart in the future for e.g. automated docker-login to the dockerhub to avoid pull rate limit.

In addition to that, the suggested change would allow simplifying manager_secret.yaml to something like:

{{- if or .Values.authSecret.enabled }} apiVersion: v1 kind: Secret metadata: name: controller-manager namespace: {{ .Release.Namespace }} type: Opaque stringData: {{ toYaml .Values.authSecret | indent 2 }} {{- end }}

// I'm aware of the redundant enabled: true added to the stringData but had no quick idea to remove that 😃

yeh, this makes more sense than the way I was doing it, I'll make the change

Patched, resolve if happy

mumoshu · 2020-12-14T00:29:34Z

Makefile

 CRD_OPTIONS ?= "crd:trivialVersions=true"

+# This is ultimately the previous tag however at the time of running the Makefile it would be the latest
+GITHUB_LATEST_TAG := $(strip $(shell git tag --sort=committerdate | tail -1))


At the time of running release.yml workflow, I think we already have the latest version tag. Probably we'd better use tail -n 2 | head -n 1?

ah, maybe this is my lack of Makefile knowledge, are the global variables not ran before any targets are ran? So we run make github-release and the global variables are populated first? As a result they are ran before the ghr ${VERSION} release/ step happens meaning the new release hasn't be created yet?

AFAIK our usage of GitHub actions workflow ensures that the job is triggered only after we manually tagged the new release.

So, the global variable is populated first, earlier than the tasks. But tail -1 would still return the tag just created manually, where you want seems like the previous one of that.

callum-tait-pbx · 2020-12-17T17:29:02Z

Moved the secret work into #240, I'll revert the secret commits on this branch

pierluigilenoci · 2021-01-19T15:11:45Z

@callum-tait-pbx @mumoshu any update?

callum-tait-pbx · 2021-01-19T15:23:39Z

tbh at this point I was waiting to see what happens with #257. I went down the Makefile route to just fall in line with the convention of the repo. I'm not actually a fan of going that direction at all so I would probably go with the #257 PR unless @mumoshu is keen on sticking with Makefiles, I was waiting for his comments tbh. The only thing I would changed about #257 is the removal of the CRDs held in the chart and instead just copy them from the config as part of the workflow because atm we have to maintain 2 copies of the same CRDs in source control for no real reason.

callum-tait-pbx · 2021-01-24T10:11:32Z

This is no longer needed now that #257 has been merged, closing.

Callum Tait and others added 12 commits December 1, 2020 15:58

feat: adding logic to include charts in release

865237f

feat: adding in linting for helm charts

64cd850

fix: removing crds from source control

48475b0

docs: adding small doc on Helm for this repo

1b87ff4

chore: reverting change made by mistake

f235947

fix: including all directories that impact Helm

3a33a52

docs: spelling / grammar

b6f2c11

fix: git doesn't like empty folders

554ecff

fix: correcting how chart compare is done

23e85dc

fix: logic was wrong

14510b2

fiz: fixing incorrect syntax

9d631be

fix: move lint into own recipe to include crds

c083dfb

callum-tait-pbx marked this pull request as ready for review December 2, 2020 23:10

Callum Tait added 4 commits December 3, 2020 15:37

fix: aligning chart version with release version

cf298e6

fix: removing v from version tag

01d70c3

Helm package would fail as the app version wasn't semantic. Stripping v from version for Helm package step

feat: adding authentication template for chart

f65c0ea

docs: adding docs for new template

68d1572

Callum Tait added 3 commits December 10, 2020 19:56

fix: incorrect case in secret namespace

63bc1a7

fix: correcting and simplifying yaml

a28d623

fix: correcting default value in values.yaml

791382a

callum-tait-pbx changed the title ~~feat/helm~~ feat: add helm package to release assets when changes occur Dec 10, 2020

callum-tait-pbx changed the title ~~feat: add helm package to release assets when changes occur~~ feat: add helm package to release assets when changes occur and add github secret to helm templates Dec 10, 2020

mumoshu reviewed Dec 14, 2020

View reviewed changes

fix: changing how secrets are deployed/configured

c74feba

callum-tait-pbx changed the title ~~feat: add helm package to release assets when changes occur and add github secret to helm templates~~ feat: add helm package to release assets when changes occur Jan 22, 2021

callum-tait-pbx closed this Jan 24, 2021

callum-tait-pbx deleted the feat/helm branch February 7, 2021 10:15

	test: create-release-dir generate fmt vet manifests helm-lint
	test: generate fmt vet manifests helm-lint

	github-release: create-release-dir release
	github-release: release

Conversation

callum-tait-pbx commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

callum-tait-pbx commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mumoshu Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

mumoshu Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

mumoshu Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

mumoshu Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

callum-tait-pbx Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

callum-tait-pbx Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

mumoshu Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

callum-tait-pbx Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

mumoshu Jan 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

callum-tait-pbx commented Dec 17, 2020

Uh oh!

pierluigilenoci commented Jan 19, 2021

Uh oh!

callum-tait-pbx commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

callum-tait-pbx commented Jan 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

callum-tait-pbx commented Dec 2, 2020 •

edited

Loading

callum-tait-pbx commented Dec 10, 2020 •

edited

Loading

mumoshu Dec 14, 2020 •

edited

Loading

mumoshu Jan 2, 2021 •

edited

Loading

callum-tait-pbx commented Jan 19, 2021 •

edited

Loading