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

Move Helm chart into tetragon directory #1845

Merged
merged 6 commits into from
Dec 8, 2023

Conversation

lambdanis
Copy link
Contributor

@lambdanis lambdanis commented Dec 5, 2023

See commits for details (and let me know if I should squash some of them). It's done mainly to make tooling (scripts) separate from the Helm chart, and also for consistency with other projects.

@lambdanis lambdanis added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note/misc This PR makes changes that have no direct user impact. area/helm Related to the Helm chart labels Dec 5, 2023
@lambdanis lambdanis requested review from a team, mtardy and willfindlay as code owners December 5, 2023 11:24
Copy link

netlify bot commented Dec 5, 2023

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 96d9807
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/6572565392f3440008756ec5
😎 Deploy Preview https://deploy-preview-1845--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@lambdanis
Copy link
Contributor Author

The links-checker CI workflow fails, because https://github.com/cilium/tetragon/tree/main/install/kubernetes/tetragon will be a thing only after this PR is merged. Maybe this doc should point to a tag instead of main branch and get updated on release. @mtardy wdyt?

@mtardy
Copy link
Member

mtardy commented Dec 7, 2023

The links-checker CI workflow fails, because https://github.com/cilium/tetragon/tree/main/install/kubernetes/tetragon will be a thing only after this PR is merged. Maybe this doc should point to a tag instead of main branch and get updated on release. @mtardy wdyt?

This kind of links (that will be created by the PR) will always fail surely. What I could do is to replace https://github.com/cilium/tetragon/tree/main/ with https://github.com/cilium/tetragon/tree/<branch for PR>/ in the link checker (and simplify the awful JSON output as well). Let me create an issue.

Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Impressed (see comments)! Thanks for the rename, hopefully nothing was forgotten and this is all smooth!

@@ -47,7 +47,7 @@ The Tetragon Helm chart source is available under
and is distributed from the Cilium helm charts repository [helm.cilium.io](https://helm.cilium.io).

To deploy Tetragon using this Helm chart you can run the following commands:
```shell-session
```shell
Copy link
Member

Choose a reason for hiding this comment

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

Thanks! well spotted, the new theme uses shell everywhere, really well spotted it just changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heh, install/kubernetes/tetragon/test.sh was actually failing without this change :)

Comment on lines +1 to +2
/install/kubernetes/tetragon/README.md linguist-generated
/docs/content/en/docs/reference/helm-chart.md linguist-generated
Copy link
Member

Choose a reason for hiding this comment

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

wow that's amazing, TIL! I explicitly excluded myself has a code owner from those files but I guess this is even better?

pushd install/kubernetes/tetragon
./test.sh
popd
./install/kubernetes/test.sh
Copy link
Member

Choose a reason for hiding this comment

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

Indeed this is no longer needed, you saw this 357206b, you have 👀 everywhere.

Moving all chart files and helper scripts one level deeper: from
install/kubernetes to install/kubernetes/tetragon. The goal is to make it
easier to develop tooling around templating the Helm chart in the future.

The only code change here is in test.sh: the path of the Helm chart docs is
prefixed by one more ../

Signed-off-by: Anna Kapuscinska <[email protected]>
Replacing all references to install/kubernetes with
install/kubernetes/tetragon:
* Makefile
* Renovate configuration
* release issue template
* Github Actions
* script updating Helm chart for release
* script for local dev installation
* documentation
* script generating Helm docs

Signed-off-by: Anna Kapuscinska <[email protected]>
There is no need to package these scripts together with the chart, so let's
move the out. I also made necessary adjustments to the scripts (paths).

Signed-off-by: Anna Kapuscinska <[email protected]>
This will mark generated files in pull requests for easier code reviews.

Signed-off-by: Anna Kapuscinska <[email protected]>
@lambdanis lambdanis force-pushed the pr/lambdanis/helmrefactor branch from c09462e to dcb5e80 Compare December 7, 2023 23:31
For some reason the Tetragon Helm chart contained the Kubeval license.
Replacing it with the correct Apache 2.0 license.

Signed-off-by: Anna Kapuscinska <[email protected]>
@lambdanis lambdanis force-pushed the pr/lambdanis/helmrefactor branch from dcb5e80 to 96d9807 Compare December 7, 2023 23:33
@lambdanis
Copy link
Contributor Author

I'm merging it, the links-checker CI job should pass on main (see #1858). cilium/charts#88 needs to follow.

@lambdanis lambdanis merged commit 9f60ecd into cilium:main Dec 8, 2023
34 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Related to the Helm chart kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants