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

Generate mainfests automatically from Helm #4278

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

lucacome
Copy link
Member

@lucacome lucacome commented Aug 22, 2023

Proposed changes

Uses Helm Charts in examples/helm-chart to template single file manifests in deploy/.

@lucacome lucacome self-assigned this Aug 22, 2023
@lucacome lucacome requested review from a team as code owners August 22, 2023 01:31
@github-actions github-actions bot added documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements helm_chart Pull requests that update the Helm Chart labels Aug 22, 2023
@lucacome lucacome force-pushed the feat/generate-manifests-from-helm branch from 8a8c717 to 19cc9bb Compare August 22, 2023 01:33
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #4278 (e7b5228) into main (7a1e600) will not change coverage.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #4278   +/-   ##
=======================================
  Coverage   51.97%   51.97%           
=======================================
  Files          59       59           
  Lines       16962    16962           
=======================================
  Hits         8816     8816           
  Misses       7849     7849           
  Partials      297      297           
Files Coverage Δ
internal/k8s/controller.go 11.93% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lucacome lucacome force-pushed the feat/generate-manifests-from-helm branch from 697a111 to 1ff8b7a Compare August 22, 2023 01:45
@github-actions github-actions bot added the tests Pull requests that update tests label Aug 23, 2023
@lucacome lucacome linked an issue Aug 23, 2023 that may be closed by this pull request
Copy link
Contributor

@ADubhlaoich ADubhlaoich left a comment

Choose a reason for hiding this comment

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

Docs-related changes LGTM!

deploy/README.md Outdated Show resolved Hide resolved
@lucacome lucacome marked this pull request as draft August 29, 2023 21:54
@lucacome lucacome force-pushed the feat/generate-manifests-from-helm branch 3 times, most recently from 3a20bd6 to 4fc8248 Compare September 26, 2023 23:50
@lucacome lucacome force-pushed the feat/generate-manifests-from-helm branch from cfd90f4 to 1512776 Compare October 16, 2023 15:48
@lucacome lucacome changed the title Refactor Helm Chart location and generate mainfests automatically Generate mainfests automatically from Helm Oct 20, 2023
@lucacome lucacome marked this pull request as ready for review October 22, 2023 10:54
Makefile Show resolved Hide resolved
@@ -100,15 +100,15 @@ CRDs](#upgrading-the-crds).
To upgrade the release `my-release`:

```console
helm upgrade my-release oci://ghcr.io/nginxinc/charts/nginx-ingress --version 1.0.1
helm upgrade my-release -n nginx-ingress oci://ghcr.io/nginxinc/charts/nginx-ingress --version 1.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

according to old command the installation was in default ns and now user is running upgrade in a different ns? I understand that these command as a guide but most users usually straight-up copy-paste and upgrade will fail due to ownership of clusterroles as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you propose we change it?

Copy link
Contributor

Choose a reason for hiding this comment

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

by leaving it to default? and giving user the option to set namespace maybe

@shaun-nx shaun-nx self-requested a review November 2, 2023 11:54
@shaun-nx shaun-nx self-requested a review November 2, 2023 15:05
@lucacome lucacome force-pushed the feat/generate-manifests-from-helm branch 4 times, most recently from 6260c58 to b0fc3f9 Compare November 2, 2023 17:57
Copy link
Contributor

@jasonwilliams14 jasonwilliams14 left a comment

Choose a reason for hiding this comment

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

looks good.
This will be great to provide a singe .yaml file for customers to use for helm upgrdes.

@lucacome lucacome force-pushed the feat/generate-manifests-from-helm branch 2 times, most recently from 4638bc0 to 2e9caa4 Compare November 2, 2023 18:48
Uses Helm Charts in examples/helm-chart to template single file
manifests in deploy/.
@lucacome lucacome force-pushed the feat/generate-manifests-from-helm branch from 2e9caa4 to e7b5228 Compare November 2, 2023 18:50
@lucacome lucacome enabled auto-merge (squash) November 2, 2023 18:50
@lucacome lucacome merged commit 1d7c8bd into main Nov 2, 2023
35 of 36 checks passed
@lucacome lucacome deleted the feat/generate-manifests-from-helm branch November 2, 2023 19:14
lucacome added a commit that referenced this pull request Nov 6, 2023
lucacome added a commit that referenced this pull request Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements helm_chart Pull requests that update the Helm Chart tests Pull requests that update tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Helm Chart and deployment manifests
5 participants