Helm chart testing by dirtycajunrice · Pull Request #648 · artifacthub/hub

dirtycajunrice · 2020-09-13T10:46:46Z

fixes #535

Separated workflow to allow filtering trigger by file changes without needing multiple step workarounds

tegioz · 2020-09-14T07:41:35Z

Could you please review this PR when you have some time? You mentioned recently #535 (comment) about using Chart Releaser as well, not sure if you'd like suggesting it as part of this or as a separate PR.

Thanks, much appreciated!

scottrigby

Thanks for the PR. This is something I planned to get to very soon. In general, have you seen https://github.com/helm/charts-repo-actions-demo?

.ct.yaml

scottrigby · 2020-09-14T14:20:15Z

.github/workflows/helm.yml

+      - '!chart/README.md'
+      - '!chart/.helmignore'


Changes to any file within the chart – including the README etc – should trigger linting. The main reason is chart version packages must be immutable, and linting checks version bumping among other things.

Ah. I have seen varying opinions on whether or not a readme change justified a version bump. Yall are the boss and i have no opinion on it haha. I will fix.

scottrigby · 2020-09-14T14:41:37Z

.ct.yaml

@@ -0,0 +1,4 @@
+helm-extra-args: --timeout 600s
+charts: chart


I would recommend changing this:

Suggested change

charts: chart

chart-dirs: chart

- charts

Mainly because the charts option "disables changed charts detection and version increment checking". Whereas chart-dirs retains those checks, which are very useful.

I would change the current chart dir to be a charts dir, and inside add another directory for the chart. Like:

. └── charts └── hub

This may seem redundant but it makes path-based checking easier. This is what I was planning to do when I opened a PR for this.

Alternatively, without changing the file tree you could set chart-dirs: "." (see helm/chart-testing#71), but you would then need to exclude all the files in the git repo root if you don't want to trigger Action runs for every change.

If you have a moment to test yourself, maybe you will be able to come to another verdict than i was able to! I have found the chart-dirs option to be extremely inconsistent and buggy when specifying anything other than charts. I attempted to use chart-dirs and point to the current directory, which failed catastrophically. I was not going to move the chart directory without the express approval of yall. Now that it is suggessted I can take care of that.

All of that said, if you know some secret about the chart-dirs function that all of my guys are oblivious to.. please drop some knowledge on me!

I haven't seen an issue for that in the chart-testing repo, but would appreciate one if you can remember problems you've seen. As an example, I can say that this does work for the stable and incubator chart repos – this is the ct config file where those dirs are defined: https://github.com/helm/charts/blob/master/test/ct.yaml#L3-L5

scottrigby · 2020-09-14T14:49:32Z

.github/workflows/helm.yml

@@ -0,0 +1,41 @@
+name: Helm CI


I'd rename this file from helm.yml to something more specific, like chart-testing.yaml, since as @tegioz said we also want to add another workflow for chart-releaser (I'll do that in a separate PR).

scottrigby · 2020-09-14T14:51:35Z

.github/workflows/helm.yml

@@ -0,0 +1,41 @@
+name: Helm CI
+on:
+  push:


Don't we instead only want to run this check when pushing to a PR fork/branch? If so, the pull_request config below should suffice. That is the most common workflow I've seen.

Ive seen unfortunate merges where master does not reflect the pr branch after merge. It was added as a "does no harm but can potentially help". Ill remove it :)

scottrigby · 2020-09-14T14:54:19Z

.github/workflows/helm.yml

+      - '!chart/.helmignore'
+  pull_request:
+    paths:
+      - 'chart/**'


This is interesting. Chart-testing already does an internal check for changed charts, but might be nice in that it would limit unneeded Action runs that just say "no chart changed detected". Would you be interested in making a PR to add this to the helm actions demo here? 😄

That is exactly the purpose. As github is undoubtedly going to enforce minutes on actions sooner rather than later, this is just a safe default

Love it. And if you feel motivated I'd support a PR to add this to the lint-test workflow in the Helm Actions demo repo 😉

scottrigby · 2020-09-14T14:56:34Z

.github/workflows/helm.yml

+      changed: ${{ steps.lint.outputs.changed }}
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v2


For the changed check, it's important to get git history, like this:

Suggested change

uses: actions/checkout@v2

uses: actions/checkout@v2

with:

fetch-depth: 0

See https://github.com/helm/charts-repo-actions-demo/blob/master/.github/workflows/lint-test.yaml#L11-L12 for reference.

I have done it a significantly more verbose way in the past. Good catch - I am going to have to go clean up a few workflows because of this

Same here 😄 You can see we updated it recently helm/charts-repo-actions-demo#25 Props to @davidkarlsen for initially pointing out the cleaner syntax in checkout action v2.

scottrigby · 2020-09-14T14:57:03Z

.github/workflows/helm.yml

+        uses: helm/[email protected]
+        with:
+          command: install
+          config: .ct.yaml


Needs newline at EOF

I am a huge proponent of pre-commit. Would you be opposed to me adding a minimal pre-commit config to the repo as well? (this would ensure the aforementioned is a non-issue, and could ensure things like sign offs are done)

I think this is up to @mattfarina, @tegioz and @cynthia-sg as maintainers of this project. I don't have an opinion.

Though this can also be caught in CI, and maybe it should? I haven't checked if the GitHub Superlinter Action does newline at EOF checks, but may be worth looking into.

I think it's better if all this kind of checks are centralized in CI.

scottrigby · 2020-09-14T15:01:30Z

.github/workflows/helm.yml

+    outputs:
+      changed: ${{ steps.lint.outputs.changed }}


Are you adding this because you have separated test into its own job? Just a note that this isn't necessary if you keep these combined into a single lint-test job. Combined may also be a little faster as well, and less repeated step definitions.

I initially had this in the same workflow as the ci.yml file. Fixing :)

scottrigby · 2020-09-14T17:39:22Z

@dirtycajunrice Looks like DCO is failing because you accepted my commit suggestion. This is a common problem on DCO-required repos. I made this browser plugin to help if you're interested: https://github.com/scottrigby/dco-gh-ui

You could also just paste your signoff line into the UI, but this helps automate that, make sure it's properly formatted etc.

scottrigby · 2020-09-14T17:41:41Z

charts/hub/ci/ct-values.yaml

@@ -0,0 +1 @@
+fullnameOverride: hub


Needs newline at EOF

Ah but also, I just said hub as an example, I see the chart name is artifact-hub. I personally think we should keep the chart directory name the same as the chart name for clarity to end users.

At risk of bikeshedding here, this brings up a question to me: should the chart name be artifact-hub or artifacthub to be consistent with the github org name? This is probably a question for @mattfarina, @tegioz and @cynthia-sg as they maintain this project.

We can rename it to artifacthub, sounds good to me 👍

to condense replies: All above issues are resolved (i believe). I also fixed the secret to match the newer template of host for postgresql. I changed the name of the chart to artifacthub as decided, and fixed the commit issue. I leave the Resolve conversation buttons to the creators so i dont step on any toes. Lemme know if you see anything else!.

Signed-off-by: Nicholas St. Germain <[email protected]>

Co-authored-by: Scott Rigby <[email protected]> Signed-off-by: Nicholas St. Germain <[email protected]>

Signed-off-by: Nicholas St. Germain <[email protected]>

…and bump Chart.yaml Signed-off-by: Nicholas St. Germain <[email protected]>

Signed-off-by: Nicholas St. Germain <[email protected]>

scottrigby

lgtm. Thanks for this 😄

tegioz · 2020-09-15T05:26:57Z

cmd/hub/Dockerfile

 USER hub
 WORKDIR /home/hub
-COPY --from=backend-builder /hub ./
+COPY --from=backend-builder /charts/artifacthub ./


Could you please leave this one as /hub? This is one of the cmd names, not the chart, and that path is where the resulting binary is located. Otherwise the binary won't be copied when building the images. Thanks!

@tegioz That was a mass sed casualty. Fixing now.

Signed-off-by: Nicholas St. Germain <[email protected]>

tegioz · 2020-09-15T06:56:56Z

Regarding the chart renaming, there are some more things we may need to update (and I was wondering if we really should change it now as part of this PR):

Chart README.md: artifact-hub is used there both for the repo and for the chart itself.
Workflow ci.yml: artifact-hub is used there too for staging and production deployments.
Charts index and packages in docs/chart directory: artifact-hub is used in all packages Chart.yaml files as well as in the index. All of them would need to be updated as well. Otherwise reindexing as is would list two different packages (with and without the dash).
The chart with the dash is also deployed in staging and production, would need to change that as well.
Once all of that is changed, in Artifact Hub, the package name artifact-hub will be deleted and a new one named artifacthub will be added. This means some entities will be deleted in cascade, like users subscriptions to that package, for example.
Links to Artifact Hub package will be broken.

Maybe we can just step back and leave the chart renaming for a different PR if we really want to go ahead with it and think about all the implications carefully. I should have thought about all of this yesterday before agreeing to the name change, my bad.

tegioz · 2020-09-15T07:07:15Z

Other than that everything looks great! So if we go back to naming the chart artifact-hub we can go ahead and merge.

Thanks for your work @dirtycajunrice 👍

…itespace / end-of-files lint on the repo Signed-off-by: Nicholas St. Germain <[email protected]>

Signed-off-by: Nicholas St. Germain <[email protected]>

scottrigby · 2020-09-15T17:13:10Z

Actually that is correct. What we would want to do before we name change is make a release where we mark the old name deprecated so it no longer appears in the hub. Then afterwards git move to the new name and "un-deprecate" the chart then so the new release appears in the hub. But yes this should probably be done in a separate PR

tegioz · 2020-09-15T18:00:19Z

Hi @dirtycajunrice

Could you please revert the mass new line change commit? It has broken some tests in the frontend as some snapshots don't match anymore.

Thanks!

dirtycajunrice · 2020-09-15T23:20:23Z

@tegioz oop went to sleep after and didnt wait for the tests. Yeah ill revert and exclude the ui

…iling-whitespace / end-of-files lint on the repo" This reverts commit 09086f6 Signed-off-by: Nicholas St. Germain <[email protected]>

Signed-off-by: Nicholas St. Germain <[email protected]>

tegioz

LGTM 👍 Thanks!

dirtycajunrice force-pushed the master branch 2 times, most recently from 15f7741 to de2bc1e Compare September 13, 2020 10:50

scottrigby suggested changes Sep 14, 2020

View reviewed changes

dirtycajunrice force-pushed the master branch 2 times, most recently from cdcced9 to 85e0287 Compare September 14, 2020 15:58

scottrigby suggested changes Sep 14, 2020

View reviewed changes

dirtycajunrice and others added 9 commits September 14, 2020 12:46

fixes artifacthub#535

9e7055a

Signed-off-by: Nicholas St. Germain <[email protected]>

add back header statements

71c98b3

Signed-off-by: Nicholas St. Germain <[email protected]>

use simple name

6f313b4

Signed-off-by: Nicholas St. Germain <[email protected]>

use v2

5e465de

Signed-off-by: Nicholas St. Germain <[email protected]>

add required config and point to chart

21d1381

Signed-off-by: Nicholas St. Germain <[email protected]>

Update .ct.yaml

5627ebe

Co-authored-by: Scott Rigby <[email protected]> Signed-off-by: Nicholas St. Germain <[email protected]>

fixing changes requested by @scottrigby

25fc7fd

Signed-off-by: Nicholas St. Germain <[email protected]>

update deployment with default host expansion, ci override for name, …

2eb1e30

…and bump Chart.yaml Signed-off-by: Nicholas St. Germain <[email protected]>

update deployment with default host expansion, ci override for name, …

0898cc4

…and bump Chart.yaml Signed-off-by: Nicholas St. Germain <[email protected]>

dirtycajunrice force-pushed the master branch from 60c9069 to 0898cc4 Compare September 14, 2020 17:49

dirtycajunrice added 2 commits September 14, 2020 13:09

change chart name, and fix secretdata to match template

f358811

Signed-off-by: Nicholas St. Germain <[email protected]>

fix remaining hosts

ed4c4f5

Signed-off-by: Nicholas St. Germain <[email protected]>

scottrigby approved these changes Sep 14, 2020

View reviewed changes

tegioz reviewed Sep 15, 2020

View reviewed changes

fix backend copy

b407650

Signed-off-by: Nicholas St. Germain <[email protected]>

dirtycajunrice requested a review from tegioz September 15, 2020 06:36

dirtycajunrice added 3 commits September 15, 2020 10:30

Merge branch 'master' into master

aae8824

change chart name back to artifact-hub, and ran a quick trailing-wh…

09086f6

…itespace / end-of-files lint on the repo Signed-off-by: Nicholas St. Germain <[email protected]>

fix ci

043d8fa

Signed-off-by: Nicholas St. Germain <[email protected]>

dirtycajunrice added 3 commits September 15, 2020 18:24

Revert "change chart name back to artifact-hub, and ran a quick tra…

d6ea6e9

…iling-whitespace / end-of-files lint on the repo" This reverts commit 09086f6 Signed-off-by: Nicholas St. Germain <[email protected]>

revert chart name change

8c89ac3

Signed-off-by: Nicholas St. Germain <[email protected]>

revert chart name change

aad5907

Signed-off-by: Nicholas St. Germain <[email protected]>

tegioz approved these changes Sep 16, 2020

View reviewed changes

tegioz merged commit 0b6750a into artifacthub:master Sep 16, 2020

		@@ -0,0 +1 @@
		fullnameOverride: hub No newline at end of file

Conversation

dirtycajunrice commented Sep 13, 2020

Uh oh!

tegioz commented Sep 14, 2020

Uh oh!

scottrigby left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottrigby commented Sep 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottrigby left a comment

Choose a reason for hiding this comment

Uh oh!

tegioz Sep 15, 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!

Choose a reason for hiding this comment

Uh oh!

tegioz commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tegioz commented Sep 15, 2020

Uh oh!

scottrigby commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tegioz commented Sep 15, 2020

Uh oh!

dirtycajunrice commented Sep 15, 2020

Uh oh!

tegioz Sep 15, 2020 •

edited

Loading

tegioz commented Sep 15, 2020 •

edited

Loading

scottrigby commented Sep 15, 2020 •

edited

Loading