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

CI: add tests & linting for helm charts #121

Merged
merged 5 commits into from
Mar 13, 2024
Merged

Conversation

ljubon
Copy link
Collaborator

@ljubon ljubon commented Oct 31, 2023

CI: add tests & linting for helm charts

Resolves: https://github.com/G-Research/oss-portfolio-maturity/issues/420

@jgiannuzzi
Copy link
Member

The tests seem to be failing 😞
@ljubon could you please fix this?

@ljubon
Copy link
Collaborator Author

ljubon commented Jan 10, 2024

The tests seem to be failing 😞 @ljubon could you please fix this?

Hi @jgiannuzzi sorry for not re-running this sooner, it seems some actions required a minor upgrade 🔧
Now it works as expected 🙏

Copy link
Member

@jgiannuzzi jgiannuzzi left a comment

Choose a reason for hiding this comment

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

could you please show a run of this workflow when a chart has changed?

.github/workflows/lint-test.yaml Outdated Show resolved Hide resolved
.github/workflows/lint-test.yaml Outdated Show resolved Hide resolved
.github/workflows/lint-test.yaml Outdated Show resolved Hide resolved
@ljubon
Copy link
Collaborator Author

ljubon commented Mar 11, 2024

could you please show a run of this workflow when a chart has changed?

Hi @jgiannuzzi, I couldn't provide old runs where I tested both no-change and change flow I'm providing you new links to the current code base:

  • Example where there is no diff between the default branch and new branch - link
  • Example where there is a change on the chart (storm) - link

@jgiannuzzi
Copy link
Member

hmm, on the "change" example, it looks like the actual lint and install validation steps are not doing anything because "No chart changes [are] detected".
https://github.com/ljubon/charts/actions/runs/8235146688/job/22518582958#step:7:17
https://github.com/ljubon/charts/actions/runs/8235146688/job/22518582958#step:9:17

@ljubon
Copy link
Collaborator Author

ljubon commented Mar 11, 2024

hmm, on the "change" example, it looks like the actual lint and install validation steps are not doing anything because "No chart changes [are] detected". https://github.com/ljubon/charts/actions/runs/8235146688/job/22518582958#step:7:17 https://github.com/ljubon/charts/actions/runs/8235146688/job/22518582958#step:9:17

The change I made was simple in template file without bumping chart version (Chart.yaml) - I'll provide in next example full chart bump example

@ljubon
Copy link
Collaborator Author

ljubon commented Mar 12, 2024

Hi @jgiannuzzi, below are the tested workflows and actual code changes that need to happen to fix reported linting errors shown in the workflow (e.g for librenms)

I would like to propose that this PR scope should focus only on introducing linting as a mechanism to prevent big changes in our charts.
In the next PR, I can include fixed errors and use this workflow to validate the change.

Below you can find the example runs as requested for the lint-test.yaml workflow

Testing results

No change to the chart (librenms & storm)

The chart librenms changed, and the version was bumped (action failed due to issues with installing the chart)

Reported errors fixed, linting success - librenms

Reported errors fixed, linting success - storm

Detected issues during installation charts librenms and storm

Storm

With the default set of values, installation failed of storm chart due to:
Failed to pull image "storm:2.4.0": rpc error: code = NotFound desc = failed to pull and unpack image "docker.io/library/storm:2.4.0": no match for platform in manifest: not found

When moved to 2.5.0 in values.yaml - it got installed with success

LibreNMS

Helm chart does not have a manifest which creates a ssd storageClassName which is required for mysql and redis installation

Copy link
Member

@jgiannuzzi jgiannuzzi left a comment

Choose a reason for hiding this comment

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

I agree with your approach. Looking forward to the chart fixes!

@ljubon ljubon merged commit b3ab697 into G-Research:master Mar 13, 2024
1 check passed
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.

2 participants