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

docs: update Helm docs for consul-k8s 1.4.0 #20770

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

zalimeni
Copy link
Member

@zalimeni zalimeni commented Feb 29, 2024

Description

This PR is the regeneration of the Helm docs with updated values from consul-k8s 1.4.0, released today.

Testing & Reproduction steps

Visual inspection

Links

Generated from the 1.4.0 values.yaml here

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@zalimeni zalimeni added type/docs Documentation needs to be created/updated/clarified pr/no-changelog PR does not need a corresponding .changelog entry backport/1.18 labels Feb 29, 2024
@zalimeni zalimeni requested a review from a team as a code owner February 29, 2024 22:27
@zalimeni zalimeni enabled auto-merge (squash) February 29, 2024 22:27
Copy link
Contributor

@boruszak boruszak left a comment

Choose a reason for hiding this comment

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

I left suggestions to correct some of the more obvious errors and deviations from the style guide. Approving on behalf of consul-docs so that you're not blocked.

Comment on lines +552 to +553
- `enableHostMetrics` ((#v-global-metrics-enablehostmetrics)) (`boolean: false`) - Configures consul agent underlying host metrics. Only applicable if
Only applicable if `global.metrics.enabled` and `global.metrics.enableAgentMetrics` is true.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `enableHostMetrics` ((#v-global-metrics-enablehostmetrics)) (`boolean: false`) - Configures consul agent underlying host metrics. Only applicable if
Only applicable if `global.metrics.enabled` and `global.metrics.enableAgentMetrics` is true.
- `enableHostMetrics` ((#v-global-metrics-enablehostmetrics)) (`boolean: false`) - Configures consul agent underlying host metrics. Default is false.
Only applicable if `global.metrics.enabled` and `global.metrics.enableAgentMetrics` is true.

I'm assuming it was meant to mirror the previous specification.

@@ -558,6 +564,120 @@ Use these links to navigate to a particular top-level stanza.
- `enableTelemetryCollector` ((#v-global-metrics-enabletelemetrycollector)) (`boolean: false`) - Configures the Helm chart’s components to forward envoy metrics for the Consul service mesh to the
consul-telemetry-collector. This includes gateway metrics and sidecar metrics.

- `prefixFilter` ((#v-global-metrics-prefixfilter)) - This configures the list of filter rules to apply for allowing/blocking
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `prefixFilter` ((#v-global-metrics-prefixfilter)) - This configures the list of filter rules to apply for allowing/blocking
- `prefixFilter` ((#v-global-metrics-prefixfilter)) - Configures the list of filter rules to apply for allowing or blocking

Comment on lines +573 to +575

- allowList:

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- allowList:

Is this an error? I know the page is automatically generated, but this seems unintentional.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think this was accidentally included


- `openMetricsPrometheus` ((#v-global-metrics-datadog-openmetricsprometheus)) - Configures Kubernetes Prometheus/OpenMetrics auto-discovery annotations for use with Datadog.
This configuration is less common and more for advanced usage with custom metrics monitoring
configurations. See https://docs.datadoghq.com/containers/kubernetes/prometheus/?tab=kubernetesadv2 for more details
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
configurations. See https://docs.datadoghq.com/containers/kubernetes/prometheus/?tab=kubernetesadv2 for more details
configurations. Refer to the [Datadog documentation](https://docs.datadoghq.com/containers/kubernetes/prometheus/?tab=kubernetesadv2) for more details.

Per style guide, links should include descriptive text.

Comment on lines +776 to +777
* `v2tenancy`:
_**Danger**_! This feature is under active development. It is not
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `v2tenancy`:
_**Danger**_! This feature is under active development. It is not
- `v2tenancy`:
_**Warning**_! This feature is under active development. It is not

Standard markdown for bullet + standard "warning" instead of "danger"

- `maxUnavailable` ((#v-server-disruptionbudget-maxunavailable)) (`integer: null`) - The maximum number of unavailable pods. By default, this will be
automatically computed based on the `server.replicas` value to be `(n/2)-1`.
- `maxUnavailable` ((#v-server-disruptionbudget-maxunavailable)) (`integer: null`) - The maximum number of unavailable pods. In most cases you should not change this as it is automatically set to
the correct number when left as null. This setting has been kept to not break backwards compatibility.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the correct number when left as null. This setting has been kept to not break backwards compatibility.
the correct number when left as null. This setting has been kept to preserve backwards compatibility.

@zalimeni zalimeni merged commit 0668ace into main Feb 29, 2024
80 of 81 checks passed
@zalimeni zalimeni deleted the zalimeni/k8s-1.4.0-helm-docs-update branch February 29, 2024 22:42
@zalimeni
Copy link
Member Author

zalimeni commented Feb 29, 2024

@boruszak apologies, these docs are auto-generated so I set the PR to merge on approval. I'll be out tomorrow but I'll make a note to follow up w/ you on these suggestions and get them committed to consul-k8s -> regenerated for this repo on Monday, if nobody else beats me to it.

cc @david-yu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-changelog PR does not need a corresponding .changelog entry type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants