Refactor lint-test.yaml GHA Workflow to use a matrix for test job generation; Add nginx.enabled and hpa.enabled tests

[jessebot]

Description of the change

• updates the test jobs to be generated from a strategy.matrix
• adds job to matrix to run a test of chart with nginx.enabled=true and image.flavor=fpm both set
• adds job to matrix to run a test of chart with hpa.enabled=true set

Benefits

• New tests are easier to add :)

• Now if anyone would like to change a config file or other template that seems like it could affect users that use the nginx.enabled parameter, this PR will do the bare minimum testing for that.

Possible drawbacks

None that I can think of, but I'm also open to feedback :)

Applicable issues

Not sure if we have any issues related to this 🤔 This is just something we've talked about several times.

Checklist

• I have read the CONTRIBUTING.md doc.
• DCO has been signed off on the commit.
• Chart version bumped in Chart.yaml according to semver.
• (optional) Parameters are documented in the README.md

[provokateurin]

How about starting to use a matrix for all these test? Currently the jobs all are almost the same, except for the chart configuration. This would make it a lot easier to add new tests and it's also more maintainable.

[jessebot]

How about starting to use a matrix for all these test? Currently the jobs all are almost the same, except for the chart configuration. This would make it a lot easier to add new tests and it's also more maintainable.

I'm not against it, but we'd have to do a bunch of conditionals for the actual install step like this one:

      - name: Run chart-testing (install with nginx.enabled)
        id: install
        if: steps.list-changed.outputs.changed == 'true'
        run: |
            ct install --target-branch ${{ github.event.repository.default_branch }} \
            --helm-extra-set-args "--set=image.flavor=fpm --set=nginx.enabled=true"

I think we would be able to do something like if: matrix.test_case == 'nginx' here? 🤔 I would have to look up the exact syntax, but I'm happy to give it a shot :)

[provokateurin]

But those arguments would just be part of the matrix

[jessebot]

oooooooo! Smart! Ok, yeah, I'll get something together in a bit and ping you again to take a look :)

[jessebot]

@provokateurin ok, now you can give it a look. I found the files tab of the PR hard to look at, so it might make more sense to check the file directly? links to import bits:

helm/.github/workflows/lint-test.yaml

Lines 59 to 79 in e3e14de

	run-tests:
	runs-on: ubuntu-22.04
	needs: [changes, lint]
	if: needs.changes.outputs.src != 'false'
	strategy:
	matrix:
	test_case:
	# test the plain helm chart with nothing changed
	- test_case: 'Default - no custom values'
	# test the helm chart with postgresql subchart enabled
	- test_case: PostgreSQL Enabled
	helm_args: '--helm-extra-set-args "--set=postgresql.enabled=true --set=postgresql.global.postgresql.auth.password=testing123456 --set=internalDatabase.enabled=false --set=externalDatabase.enabled=True --set=externalDatabase.type=postgresql --set=externalDatabase.password=testing12345"'
	# test the helm chart with nginx container enabled
	- test_case: Nginx Enabled
	helm_args: '--helm-extra-set-args "--set=image.flavor=fpm --set=nginx.enabled=true"'
	# test the helm chart with horizontal pod autoscaling enabled
	- test_case: Horizontal Pod Autoscaling Enabled
	helm_args: '--helm-extra-set-args "--set=image.flavor=fpm --set=nginx.enabled=true"'

helm/.github/workflows/lint-test.yaml

Lines 111 to 119 in e3e14de

	- name: Run chart-testing (install ${{ matrix.test_case }})
	id: install
	if: steps.list-changed.outputs.changed == 'true'
	run: |
	if [[ ${{ matrix.test_case }} == 'Default - no custom values' ]]; then
	ct install --target-branch ${{ github.event.repository.default_branch }}
	else
	ct install --target-branch ${{ github.event.repository.default_branch }} ${{ matrix.helm_args }}
	fi

[jessebot]

We could merge this, then I can rebase the hpa PR and add its test the same way, so that way we can get some immediate testing of it all?

[jessebot]

oh wait, you'd need to change the required tests again to be run-tests. That's why the checks are upset, I think.

[provokateurin]

Awesome, seems to work ❤️

Please remove the test commit and then I will change the settings to allow this to be merged

[provokateurin]

Maybe also squash the commits, then the history is a bit cleaner

[provokateurin]

I hope this works now, maybe I have to adjust the settings to require the individual jobs.

[jessebot]

Ok, I made it all one commit, however, I just noticed something off 🤔

All the jobs generated on my last commit test commit have weird names:
https://github.com/nextcloud/helm/actions/runs/10061297081

for instance the postgres job here is called:
[run-tests (PostgreSQL Enabled, --helm-extra-set-args "--set=postgresql.enabled=true --set=postgre...](https://github.com/nextcloud/helm/actions/runs/10061297081/job/27811245781#logs)

The actual args didn't template here either 😞 :
https://github.com/nextcloud/helm/actions/runs/10061297081/job/27811245781#step:8:1

[provokateurin]

Yeah that is automatic from the matrix, nothing we can do about

[provokateurin]

Alright I have to require all matrix tests individually

[provokateurin]

Some more testing of this PR would be nice:

• No changes
• Valid changes
• Invalid changes
  Just to be sure we don't end up with broken CI afterwards.

[jessebot]

Some more testing of this PR would be nice:

* No changes

* Valid changes

* Invalid changes
  Just to be sure we don't end up with broken CI afterwards.

1. No changes would be the current state it's in, but it still needs you to re-require the summary job, I think?

2. valid changes can be tested after we fix the required jobs and we'll do the same test commit again?

3. invalid changes are harder to test, but I'm open to suggestions 🤷

[provokateurin]

I think invalid changes you can just force by breaking the yaml syntax, then the tests should definitely fail

[jessebot]

I think invalid changes you can just force by breaking the yaml syntax, then the tests should definitely fail

I can try that, but it might be caught by the lint before it gets to actually running the other jobs 🤔 We'll try after valid changes though 🫡

[provokateurin]

Or you can change the image tag to some version that doesn't exist, then the lint will pass but the tests won't

[jessebot]

ok, lit, the no changes is completely valid, finally!
https://github.com/nextcloud/helm/actions/runs/10062423969/job/27814902243

I will push up a test commit that should be valid for the second wave of tests :)

[jessebot]

They all failed on creating the kind cluster? 🤔 could this be because we changed the runs-on?

[provokateurin]

Yeah, it could be that this doesn't work on the self-hosted runners. Just change the tests back to the previous runs-on and we should be good.

[jessebot]

Looks like it works with a valid change 👍 (bump chart version and remove an empty commented line in values.yaml):
https://github.com/nextcloud/helm/actions/runs/10062658048/job/27815741344?pr=598#step:8:1

I will now to try to make an invalid change, by changing the image.tag to testing123.

[provokateurin]

We should set fail-fast: false on the matrix so that one failed job doesn't kill the other jobs (which might work just fine).

[jessebot]

Okay, so for invalid change, the first test failed expectedly:
https://github.com/nextcloud/helm/actions/runs/10063299174/job/27817817944?pr=598#step:8:129

However, unexpectedly, the rest were canceled 🤷 I guess they were all gonna fail the same way anyway, but I would have expected the other jobs to start, unless they were canceled by a maintainer or member? Do we want to do further testing of this, or would you say we're good to go on testing for now?

[jessebot]

We should set fail-fast: false on the matrix so that one failed job doesn't kill the other jobs (which might work just fine).

oh, I didn't refresh the page before posting again 🤦 but the answer to your request is yes, I can do that!

[jessebot]

Looks like they all failed appropriately :)
https://github.com/nextcloud/helm/actions/runs/10063649895/job/27818889110?pr=598#step:8:1

I'll remove the test changes now

[provokateurin]

Perfect! One more squash please :)

[jessebot]

I have made this all one commit again and made sure to keep your coauthor line in the commit message :) Thanks again for all your help here!

[provokateurin]

Send it!