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

Helm improvments #85

Merged
merged 2 commits into from
Jun 26, 2023
Merged

Helm improvments #85

merged 2 commits into from
Jun 26, 2023

Conversation

fmuyassarov
Copy link
Collaborator

This PR adds a GitHub action for linting Helm charts on pull requests and fixes minor issue with namespaces in the Helm chart installation guide.

Helm linter is basically validating helm chart values in a chart's values.yaml file with JSON schemas.

@fmuyassarov
Copy link
Collaborator Author

Here is the GitHub action result of the first linter run: https://github.com/containers/nri-plugins/actions/runs/5377726551/jobs/9756576779?pr=85

This commit adds a simple github action which runs helm lint for
balloons and topology-aware plugins.

Signed-off-by: Feruzjon Muyassarov <[email protected]>
When installing the Helm chart, it is important to explicitly
specify the desired namespace for installation. It should be noted
that failing to specify the namespace will result in the Helm
chart installing the manifests in the default namespace.
Signed-off-by: Feruzjon Muyassarov <[email protected]>
Copy link
Collaborator

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

LGTM

@klihub
Copy link
Collaborator

klihub commented Jun 26, 2023

@fmuyassarov The PR as such looks fine to me. That said, I have a tangentially related question now that I look at the changes: Shouldn't/couldn't we, please, have a helm chart for the template policy as well ?

One of the main points of the template policy is that it serves as a code monkey see, code monkey do-template for folks to bootstrap developing their own custom policy. This should happen without first having to understand all the bits and pieces in this repo to get things right/up and running. And IMO, this should cover anything related to the policy , including deployment, helm charts, and even documentation.

@marquiz, @jukkar WDYT ?

@klihub klihub requested a review from jukkar June 26, 2023 12:53
Copy link
Collaborator

@klihub klihub left a comment

Choose a reason for hiding this comment

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

This LGTM, but please @jukkar, @fmuyassarov take a look at a my related question.

@fmuyassarov
Copy link
Collaborator Author

@fmuyassarov The PR as such looks fine to me. That said, I have a tangentially related question now that I look at the changes: Shouldn't/couldn't we, please, have a helm chart for the template policy as well ?

One of the main points of the template policy is that it serves as a code monkey see, code monkey do-template for folks to bootstrap developing their own custom policy. This should happen without first having to understand all the bits and pieces in this repo to get things right/up and running. And IMO, this should cover anything related to the policy , including deployment, helm charts, and even documentation.

@marquiz, @jukkar WDYT ?

I can definitely add chart for the template policy + kustomize installation. Would you like that to happen on this PR or shall I send a seperate PR for that (personally I prefer to have separate PR)?

@jukkar
Copy link
Collaborator

jukkar commented Jun 26, 2023

Shouldn't/couldn't we, please, have a helm chart for the template policy as well ?

Makes sense, +1 to this.

Copy link
Collaborator

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

LGTM, I am ok if the template changes are in separate pr.

@klihub
Copy link
Collaborator

klihub commented Jun 26, 2023

Yes, I also think separate PR is fine.

@klihub klihub merged commit 44ef13a into containers:main Jun 26, 2023
3 checks passed
@fmuyassarov fmuyassarov deleted the ga-helm-linter branch June 26, 2023 19:25
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.

3 participants