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

Static helm #320

Merged
merged 5 commits into from
Aug 5, 2024
Merged

Static helm #320

merged 5 commits into from
Aug 5, 2024

Conversation

sgburtsev
Copy link
Contributor

As suggested in #313 (comment) I got rid of the helmify step.

The resulting CRDs are almost identical, except for the wrapping of long lines and missing labels (which are not necessary).

@sgburtsev
Copy link
Contributor Author

sgburtsev commented Aug 2, 2024

@l0kix2, could you try to restart the test? It shouldn't be affected by my changes.

Copy link
Collaborator

@l0kix2 l0kix2 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, helm chart looks almost the same, which is we wanted
I only not sure why those CERTIFICATE_NAMESPACE variables are replaced

Makefile Outdated
$(call go-install-tool,$(ENVSUBST),github.com/a8m/envsubst/cmd/envsubst,$(ENVSUBST_VERSION))

.PHONY: kubectl-slice
kubectl-slice: $(KUBECTL_SLICE) ## Download yq locally if necessary.
Copy link
Collaborator

Choose a reason for hiding this comment

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

yq --> kubectl-slice

config/crd/patches/cainjection_in_chyts.yaml Show resolved Hide resolved
@l0kix2 l0kix2 merged commit 7ec82a6 into ytsaurus:main Aug 5, 2024
5 checks passed
@sgburtsev sgburtsev deleted the static-helm branch August 5, 2024 14:52
leo-astorsky pushed a commit to leo-astorsky/yt-k8s-operator that referenced this pull request Aug 13, 2024
* Fix kustomization deprecation warnings

* Update helmify

* Fix CRDs patches

* Get rid of helmify, copy generated CRDs only
@wilwell
Copy link
Contributor

wilwell commented Sep 9, 2024

@sgburtsev, it looks like this change has broken the Helm chart construction.

If you look at the Helm chart created before and after your change, it's clear that your change has affected the generation of multiple YAML files.

For example, deployment.yaml is missing now. It only generates templates/crds files.

Could you please take a look? Maybe you understand why your change triggered this behavior?

@wilwell
Copy link
Contributor

wilwell commented Sep 9, 2024

I've fixed this issue in 5863668, but there is still one more bug.

In the files values.yaml the image tag of ytsaurus/k8s-operator is always 0.0.0-alpha, even if you redefine RELEASE_VERSION with another value.

@sgburtsev do you know how to fix that?

@sgburtsev
Copy link
Contributor Author

Unfortunately, I didn't take into account github workflows after this changes.
I will take a look, but the main purpose of the PR was exactly what you said: to generate only CRDs and leave other as static.

@wilwell
Copy link
Contributor

wilwell commented Sep 10, 2024

How to reproduce the behaviour:

In make file redefine release version, e.g. RELEASE_VERSION = "1.1.1"

run 2 commands from release in Makefile:

$(MAKE) helm-chart
helm package $(OPERATOR_CHART)

If you unpack generated helm package, you find that image tag of ytsaurus/k8s-operator is 0.0.0-alpha.


Moreover: if you remove tag: 0.0.0-alpha from values.yaml in ytop-chart from repo and then regenerate helm package, you will find that tage field is absent in the generated package too.

I suppose that new helm-chart command just copies values.yaml from ytop-chart in repo (!?)

@sgburtsev
Copy link
Contributor Author

It's quite a discovery for me that nightly build has another name. I supposed that a nightly and a release build should be completely interchangeable. @l0kix2, do you know the reason for that?
As for the tag for releases, it could be easily fixed without need of generating values. I will prepare a fix ASAP.
The motivation of moving from a generated Chart to a static one is in #313.

@l0kix2
Copy link
Collaborator

l0kix2 commented Sep 10, 2024

It's quite a discovery for me that nightly build has another name. I supposed that a nightly and a release build should be completely interchangeable. @l0kix2, do you know the reason for that?

I don't know the answer. I don't think we expect them to be different in any way, so I would suggest we have one image name for operator and helm chart with either release (like 0.0.15) or dev (like 0.0.428-dev-5863668ed96723dba96eff8b2ed81dd001ea3dda) version part.

@savnadya do we have a good reason to have different names here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants