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

feat(ci): workflow to lint and test charts #73

Merged
merged 4 commits into from Jun 5, 2023
Merged

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Jun 1, 2023

Fixes #72

  • Restructure chart directory
  • Add CI jobs to lint and test helm releases. Test procedure includes:
    • Upgrade test (i.e. install version on target branch then try to upgrade to any new version by PR/push).
    • Helm test (i.e. one with annotation helm.sh/hook: test under charts/cryostat/templates/tests).

References:

@tthvo tthvo added feat New feature or request ci labels Jun 1, 2023
@tthvo tthvo requested review from andrewazores and ebaron June 1, 2023 02:03
@tthvo
Copy link
Member Author

tthvo commented Jun 1, 2023

https://redhat-cop.github.io/ci/linting-testing-helm-charts.html

The charts/*/ci/ Folder

This folder is for testing different values files during the CI process. These values merge with the chart’s values.yaml file and applied to ensure that different combinations of values still result in a successful chart installation across changes. You can create as many values files as you want under this folder using whatever filename you think is necessary to represent the test case. Here’s an example:

charts/my-chart/ci/nodeport-values.yaml
charts/my-chart/ci/pvc-values.yaml

Seems like we can add partial values.yaml (merged with default values) to configure test cases. What test cases do you think we should run? Right now, job only uses the default values.

@ebaron
Copy link
Member

ebaron commented Jun 1, 2023

https://redhat-cop.github.io/ci/linting-testing-helm-charts.html

The charts/*/ci/ Folder
This folder is for testing different values files during the CI process. These values merge with the chart’s values.yaml file and applied to ensure that different combinations of values still result in a successful chart installation across changes. You can create as many values files as you want under this folder using whatever filename you think is necessary to represent the test case. Here’s an example:
charts/my-chart/ci/nodeport-values.yaml
charts/my-chart/ci/pvc-values.yaml

Seems like we can add partial values.yaml (merged with default values) to configure test cases. What test cases do you think we should run? Right now, job only uses the default values.

I think the most obvious one is minimal and non-minimal. Testing the different networking types could be useful, but I'm not sure if it would test any different code paths with the current test.

@tthvo
Copy link
Member Author

tthvo commented Jun 1, 2023

I think the most obvious one is minimal and non-minimal. Testing the different networking types could be useful, but I'm not sure if it would test any different code paths with the current test.

Should this be a separate issue when the test is updated then?

@ebaron
Copy link
Member

ebaron commented Jun 1, 2023

I think the most obvious one is minimal and non-minimal. Testing the different networking types could be useful, but I'm not sure if it would test any different code paths with the current test.

Should this be a separate issue when the test is updated then?

I'm fine with that. For now minimal & non-minimal seems good.

@tthvo
Copy link
Member Author

tthvo commented Jun 1, 2023

Updated to include minimal and non-minimal test scenarios :))

One thing: Partial value files under charts/cryostat/ci/ must be named <anything>-values.yaml for the helm testing tool to pick up despite the guide says "whatever filename".

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Hey @tthvo, I noticed one small thing in the workflow file. Also do you have any test CI runs for this?

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@tthvo
Copy link
Member Author

tthvo commented Jun 5, 2023

Also do you have any test CI runs for this?

I think because it targets event pull_request so the workflow actually runs on this PR too.

https://github.com/cryostatio/cryostat-helm/actions/runs/5150101449/jobs/9273872792?pr=73

@tthvo
Copy link
Member Author

tthvo commented Jun 5, 2023

This PR CI just failed testing once: https://github.com/cryostatio/cryostat-helm/actions/runs/5179579235/attempts/1 but passed later. Might be flanky at times...

@ebaron
Copy link
Member

ebaron commented Jun 5, 2023

This PR CI just failed testing once: https://github.com/cryostatio/cryostat-helm/actions/runs/5179579235/attempts/1 but passed later. Might be flanky at times...

Oh right! So used to the pull_request_target ones with secrets. I'll take a look at the failure. Maybe it's a timeout we can extend somewhere.

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

I reran the checks a few more times and never encountered a failure. Maybe something weird due to the PR. Let's merge this and deal with it if we see it happen again. Thanks @tthvo!

@ebaron ebaron merged commit adeac87 into cryostatio:main Jun 5, 2023
5 checks passed
@tthvo tthvo deleted the ci branch June 5, 2023 21:17
mergify bot pushed a commit that referenced this pull request Sep 11, 2023
* chore(chart): put cryostat chart into charts directory

* feat(ci): workflow to lint and test charts

https://redhat-cop.github.io/ci/linting-testing-helm-charts.html

* feat(ci): add partial values for test scenarios

* chore(ci): use ref name without sub

(cherry picked from commit adeac87)
ebaron pushed a commit that referenced this pull request Sep 11, 2023
* chore(chart): put cryostat chart into charts directory

* feat(ci): workflow to lint and test charts

https://redhat-cop.github.io/ci/linting-testing-helm-charts.html

* feat(ci): add partial values for test scenarios

* chore(ci): use ref name without sub

(cherry picked from commit adeac87)

Co-authored-by: Thuan Vo <thvo@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Request] CI test lint and test chart
2 participants