Skip to content

Conversation

@shainaraskas
Copy link
Collaborator

@shainaraskas shainaraskas commented Oct 30, 2025

The helm chart version is not static and often gets bumped with an ECK release.

this PR skips the value completely so we no longer need to worry about it being out of date

@shainaraskas shainaraskas requested review from a team as code owners October 30, 2025 20:25
@github-actions
Copy link

github-actions bot commented Oct 30, 2025

🔍 Preview links for changed docs

Copy link
Contributor

@rhr323 rhr323 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the improvement!

@eedugon
Copy link
Contributor

eedugon commented Oct 31, 2025

IMO the usage we do of the eck_helm_chart_version doesn't justify the creation of the substitution.

We are using it only in an example for illustration purposes of how the Ingress would look like, and the important part of the content is the Ingress payload itself, not really the labels. So, the focus should be on the spec and rules of the content, and not on the labels (I'm sorry because I introduced these examples).

So, if we want this substitution only to solve this, I think it would be better to remove the labels from the illustrative examples instead of creating a new subs that we will have to maintain.

But of course, if there's benefit on mentioning on our docs the latest helm chart release and explain if different helm chart releases support different things (maybe in the future), then we would definitely need this.

WDYT @rhr323 or @barkbay ?

I think the doc would still be ok if we leave something like:

metadata:
  name: elasticsearch
  labels:
    ...
    ...
spec:

PS - Note that when I added the ingress example I did it because it's useful to see how an Ingress object for an ECK-managed resource could look like in case a user wants to create ingress objects manually, which is a common use case also.

@shainaraskas
Copy link
Collaborator Author

@eedugon great points. will let the devs weigh in on what they think is best.

Copy link
Contributor

@barkbay barkbay left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, this one fell through the cracks.

LGTM, left a small comment about naming.

Copy link
Contributor

@barkbay barkbay left a comment

Choose a reason for hiding this comment

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

Sorry I read the PR content first, and missed @eedugon's comment 🙃

I think it would be better to remove the labels from the illustrative examples instead of creating a new subs that we will have to maintain.

No strong opinion, but what you said makes sense to me.These labels don’t seem to add much value; we could indeed consider removing them.

@shainaraskas shainaraskas changed the title Introduce helm chart version variable remove references to stack helm chart version Nov 5, 2025
@shainaraskas
Copy link
Collaborator Author

@barkbay I took @eedugon's suggestion and cut this out to reduce the number of things to update as part of a release.

@shainaraskas shainaraskas requested a review from eedugon November 5, 2025 20:59
Copy link
Contributor

@yetanothertw yetanothertw left a comment

Choose a reason for hiding this comment

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

LGTM 🐈

Copy link
Contributor

@barkbay barkbay left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@eedugon eedugon left a comment

Choose a reason for hiding this comment

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

LGTM!

@shainaraskas shainaraskas enabled auto-merge (squash) November 10, 2025 16:10
@shainaraskas shainaraskas merged commit 95a7dda into main Nov 10, 2025
7 checks passed
@shainaraskas shainaraskas deleted the eck-helm-variable branch November 10, 2025 16:13
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.

7 participants