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 nightly release #345

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

@RafalKorepta RafalKorepta force-pushed the rk/k8s-440/helm-nightly-release branch 5 times, most recently from 1abe2b1 to 86d064d Compare December 6, 2024 14:24
@@ -36,3 +36,9 @@ operator/tests/e2e-v2-helm/

# Go releaser artifacts
dist/

# Dep Chart Lock file
Chart.lock
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we determine that we need Chart.lock to be checked in? https://github.com/redpanda-data/helm-charts/blob/main/charts/redpanda/Chart.lock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I'm going back and fourth with this. Our tests in redpanda repo requires Chart.lock to have stable input. I didn't want operator helm chart lock as I feel like it's not required. Never the less if .helmignore will exclude Chart.lock I can commit Chart.lock.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't currently have Chart.lock in our .helmignore either. That also caused problems.

charts/connectors/Chart.yaml Outdated Show resolved Hide resolved
operator/go.mod Outdated Show resolved Hide resolved
- helm registry login registry-1.docker.io -u {{.DOCKERHUB_USER}} --password {{.DOCKERHUB_TOKEN}}
- helm dep update charts/operator
# To change name of the operator helm chart malformer program is executed
- go run ./nightly-malformer/...
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you could accomplish this by just running helm package --version $TAG_NAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The most important change is changing the Chart name just to match the https://hub.docker.com/r/redpandadata/redpanda-operator-nightly docker hub repository. That way we can host helm chart and containers under one repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not to be too crotchety but it looks like this could also be handled by a tiny bit of scripting with tar and yq. Doing so would be more easily reusable for other charts and not dependent on go loading Chart.yaml.

taskfiles/k8s.yml Outdated Show resolved Hide resolved
taskfiles/k8s.yml Show resolved Hide resolved
- helm dep update charts/operator
# To change name of the operator helm chart malformer program is executed
- go run ./nightly-malformer/...
- helm package charts/operator
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: To avoid littering the repo when running this task, it'd be preferable to output the build to either a tempdir or the dist directory which is git ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. will adjust.

My personal goal would be to use go program to load chart and the push it to docker hub. I'm using helm cli just for the quick turnaround.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might be able to fix that with some help from dev prod? If we can update the k8s-builders to use the containerd image store by default multi platform builds will "just work"1.

Footnotes

  1. https://docs.docker.com/engine/storage/containerd/

@RafalKorepta RafalKorepta force-pushed the rk/k8s-440/helm-nightly-release branch from 8af6cca to 5f19af1 Compare December 12, 2024 10:39
@@ -36,3 +36,9 @@ operator/tests/e2e-v2-helm/

# Go releaser artifacts
dist/

# Dep Chart Lock file
Chart.lock
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't currently have Chart.lock in our .helmignore either. That also caused problems.

@@ -36,7 +36,7 @@ kubeVersion: ">= 1.25.0-0"

icon: https://images.ctfassets.net/paqvtpyf8rwu/3cYHw5UzhXCbKuR24GDFGO/73fb682e6157d11c10d5b2b5da1d5af0/skate-stand-panda.svg
sources:
- https://github.com/redpanda-data/helm-charts
- https://github.com/redpanda-data/redpanda-operator/charts
Copy link
Contributor

Choose a reason for hiding this comment

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

These links don't resolve as they need the ref set. https://github.com/redpanda-data/redpanda-operator/tree/main/charts

Should we go ahead and link to the specific folder or the chart as well? e.g. https://github.com/redpanda-data/redpanda-operator/tree/main/charts/console

func ChartMeta() helmette.Chart {
return chartMeta
}
// render is the entrypoint to both the go and helm versions of the redpanda
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to not merge changes to the charts until we have CI running generation and unit tests for them. It's too easy for something to accidentally slip in and not get caught until MUCH later.

- helm registry login registry-1.docker.io -u {{.DOCKERHUB_USER}} --password {{.DOCKERHUB_TOKEN}}
- helm dep update charts/operator
# To change name of the operator helm chart malformer program is executed
- go run ./nightly-malformer/...
Copy link
Contributor

Choose a reason for hiding this comment

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

Not to be too crotchety but it looks like this could also be handled by a tiny bit of scripting with tar and yq. Doing so would be more easily reusable for other charts and not dependent on go loading Chart.yaml.

@@ -501,6 +501,7 @@ func (r *StatefulSetResource) obj(
{
Name: configuratorContainerName,
Image: r.fullConfiguratorImage(),
Command: []string{"/redpanda-operator", "configure"},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change has leaked into the wrong commit?

@@ -70,25 +70,26 @@ tasks:
- |
go test -v ./... -coverprofile cover.out -timeout 20m

build-operator-images:
build-container-image:
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these changes related to the nightly helm release or the removal of the vectorized repository? If it's the nightly helm release, could you explain why this change is needed? It looks to primarily be a refactor to me?

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