eck webhook default value clarified#6734
Conversation
Elastic Docs AI PR menuCheck the box to run an AI review for this pull request.
Powered by GitHub Agentic Workflows and docs-actions. For more information, reach out to the docs team. |
🔍 Preview links for changed docs |
Elastic Docs Style Checker (Vale)Summary: 2 suggestions found 💡 Suggestions (2): Optional style improvements. Apply when helpful.
The Vale linter checks documentation changes against the Elastic Docs style guide. To use Vale locally or report issues, refer to Elastic style guide for Vale. |
| | `webhook-port` | 9443 | Port to listen for incoming validation requests. | | ||
|
|
||
| :::::{note} | ||
| Although the `enable-webhook` flag default is `false`, the default ECK installation manifests and Helm chart set it to `true`. |
There was a problem hiding this comment.
can we remove "default" later in this sentence? we might rephrase to say "if you're editing x y z / installed using x y z methods, the setting is already present and set to true"
I think a think that makes this confusing is we call this manual configuration but likely it's being applied in a layer over one of these defaults. we also say "If you installed ECK without the webhook" but what is the context that makes this possible?
consider putting this default mismatch info directly in the description rather than as a note below the table
There was a problem hiding this comment.
"If you installed ECK without the webhook" but what is the context that makes this possible?
You can install ECK without the webhook by tweaking the operator.yaml manifest file before installing through a manifest, or by tweaking the values.yaml before installing the helm chart. When doing that, you can decide to either "remove the webhook setting" (it would then default to false :) ), or set it to false explicitly.
I think it's a good practice when installing a production system to decide what exact configuration you want and not just deploy the quickstart or the default provided file, and some users could have installed ECK without the webhook.
Anyway, this PR aims to solve a possible confusion due to the fact that we provide default config files enabling the webhook but internally the code has a default value of disabling it. That can generates a bit of confusion when reading both sentences.
We can rephrase it to something like:
Although the
enable-webhookflag of the operator defaults tofalsewhen not provided, the standard ECK installation manifests and Helm chart set it totrueexplicitly.
But please suggest whatever you think it's better :)
What I want to ensure is that people doesn't get confused when reading these two sentences:
- The webhook is installed by default (through our manifests)
enable-webhookdefault value is false
Oh, and if you believe we can remove the note completely because the other minor change we did already avoids the possible confusion then I'm totally ok with it too ;)
There was a problem hiding this comment.
that rephrase looks better to me.
There was a problem hiding this comment.
Instead of the external "note", I've added the sentence directly in the table, in the place where the confusion could occur. I think it's a better approach.
Sorry I requested a new review :) rephrase included but directly within the table. I think it's nicer and less disruptive. |
shainaraskas
left a comment
shainaraskas
left a comment
There was a problem hiding this comment.
approved with a small suggest
Co-authored-by: shainaraskas <58563081+shainaraskas@users.noreply.github.com>
2 participants
Summary
Closes #6160
Clarifies a bit better the default value of
falseof enable-webhook at code level in ECK, while at the same time our default installation enables it by default.