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.

Sign up for GitHub

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

Jump to bottom

Make helm charts consistent with how fields in spec are handled. (kibana only) #8192

Merged
merged 9 commits into from
Nov 22, 2024
Merged

Make helm charts consistent with how fields in spec are handled. (kibana only) #8192

merged 9 commits into from
Nov 22, 2024

Conversation

naemono
Copy link
Contributor

@naemono naemono commented Nov 11, 2024 •
edited
Loading

related: #8184

Details of suggested change

It was pointed out in the above PR that the helm charts aren't consistent in how they implement values that end up within the spec field of the associated resource. Unfortunately the changes in 8184 are not backwards compatible, and are a breaking change. I went down the path of determining what a non-breaking change would look like, and just implemented this in the eck-kibana chart.

The question I have:

  • Is this a path we want to go down to make the charts more consistent? Because this makes the templates more complex, but on the plus side, it significantly increases the test coverage.

(The intent is to do this for all inconsistent charts; just wanted to make the initial review/discussions easier)

Implementation Note

  1. The way this is implemented makes the previous behavior (having .spec.field in the values take precedence over just field in the values.
  2. Mixing these 2 forms of values (some in spec in the values file, some not in spec) likely has unpredictable behavior. (for instance setting spec.http.something, and also setting http.somethingElse)

@naemono
Make helm charts consistent.
7c59a77 
Signed-off-by: Michael Montgomery <[email protected]>
@naemono naemono added discuss We need to figure this out >refactoring labels Nov 11, 2024
@pebrc
Copy link
Collaborator

pebrc commented Nov 14, 2024

I think your approach is reasonable. I think it is key that we don't have breaking changes. I don't think there is any tolerance on the user side for breaking changes in Helm charts.
But consistency is also key. AFAICT only Elasticsearch, Logstash and EnterpriseSearch follow the structured approach, all the others just have .spec

@naemono
Copy link
Contributor Author

naemono commented Nov 18, 2024

I think your approach is reasonable. I think it is key that we don't have breaking changes. I don't think there is any tolerance on the user side for breaking changes in Helm charts. But consistency is also key. AFAICT only Elasticsearch, Logstash and EnterpriseSearch follow the structured approach, all the others just have .spec

Since this is the case, I'll mark this as ready for review to get additional feedback. ty.

@naemono naemono marked this pull request as ready for review November 18, 2024 15:16
@naemono naemono changed the title Discuss: Make helm charts consistent with how fields in spec are handled. (kibana only) Make helm charts consistent with how fields in spec are handled. (kibana only) Nov 18, 2024
kvalliyurnatt
kvalliyurnatt reviewed Nov 20, 2024
View reviewed changes
deploy/eck-stack/charts/eck-kibana/examples/http-configuration.yaml
@@ -13,25 +13,24 @@ labels: {}
annotations: {}
# key: value

spec:
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a comment saying this spec. is deprecated and config. is the preferred way to set these configs. Not sure what the right place is to add this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kvalliyurnatt I adjusted this in f7b341c. Let me know if the wording works for you. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, LGTM.

@naemono
Add deprecation notice.
f7b341c 
Add link to kibana config.
Adjust comments.

Signed-off-by: Michael Montgomery <[email protected]>
@naemono
Show full image path in values for kibana.
d66e820 
Signed-off-by: Michael Montgomery <[email protected]>
@thbkrkr
Copy link
Contributor

thbkrkr commented Nov 20, 2024 •
edited
Loading

We probably also want to refactor deploy/eck-stack/examples/kibana/.

@naemono
Copy link
Contributor Author

naemono commented Nov 20, 2024

We probably also want to refactor deploy/eck-stack/examples /kibana/.

great catch! will.do

@naemono
Copy link
Contributor Author

naemono commented Nov 20, 2024

We probably also want to refactor deploy/eck-stack/examples /kibana/.

@naemono naemono closed this Nov 20, 2024
@naemono naemono reopened this Nov 20, 2024
@naemono
Copy link
Contributor Author

naemono commented Nov 20, 2024

We probably also want to refactor deploy/eck-stack/examples /kibana/.

Sorry, hit the wrong button and closed this....

@naemono
Also fix eck-stack/examples.
ecb9db3 
Signed-off-by: Michael Montgomery <[email protected]>
@naemono
Copy link
Contributor Author

naemono commented Nov 20, 2024

We probably also want to refactor deploy/eck-stack/examples /kibana/.

great catch! will.do

@thbkrkr this has been fixed.

thbkrkr
thbkrkr approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@thbkrkr thbkrkr left a comment

Choose a reason for hiding this comment

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

LGTM

@naemono
Fix kibana monitoring rendering.
575d42b 
Add kibana monitoring rendering tests.

Signed-off-by: Michael Montgomery <[email protected]>
@naemono
Default count to 1
002018a 
Signed-off-by: Michael Montgomery <[email protected]>
@naemono naemono mentioned this pull request Nov 20, 2024
Merged
@naemono
remove default for count.
4410243 
Signed-off-by: Michael Montgomery <[email protected]>
@naemono naemono mentioned this pull request Nov 21, 2024
Merged
@naemono naemono merged commit a79db96 into elastic:main Nov 22, 2024
5 checks passed
@naemono naemono deleted the helm-chart-image-fix-01 branch November 22, 2024 16:55
@thbkrkr thbkrkr mentioned this pull request Nov 25, 2024
Closed
@naemono naemono mentioned this pull request Dec 3, 2024
Merged
naemono added a commit to naemono/cloud-on-k8s that referenced this pull request Dec 3, 2024
@naemono @thbkrkr
Make helm charts consistent with how fields in spec are handled. (k…
f7dd90f 
…ibana only) (elastic#8192)

* Make helm charts consistent with how .spec is handled.

---------

Signed-off-by: Michael Montgomery <[email protected]>
Co-authored-by: Thibault Richard <[email protected]>
(cherry picked from commit a79db96)
Signed-off-by: Michael Montgomery <[email protected]>
@naemono
Copy link
Contributor Author

naemono commented Dec 3, 2024

💚 All backports created successfully

Status Branch Result
✅ 2.16

Questions ?

Please refer to the Backport tool documentation

@naemono naemono mentioned this pull request Dec 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvalliyurnatt kvalliyurnatt kvalliyurnatt approved these changes

@thbkrkr thbkrkr thbkrkr approved these changes

@pebrc pebrc Awaiting requested review from pebrc

@barkbay barkbay Awaiting requested review from barkbay

@rhr323 rhr323 Awaiting requested review from rhr323

Assignees
No one assigned
Labels
discuss We need to figure this out :helm-charts >refactoring v2.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@naemono @pebrc @thbkrkr @kvalliyurnatt