-
Notifications
You must be signed in to change notification settings - Fork 380
NO-JIRA: clarify ExternalLabels doc #2691
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
base: main
Are you sure you want to change the base?
Conversation
@machine424: This pull request explicitly references no jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: machine424 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pkg/manifests/types.go
Outdated
// The default value is empty, which indicates no limit. | ||
EnforcedBodySizeLimit string `json:"enforcedBodySizeLimit,omitempty"` | ||
// Defines labels to be added to any time series or alerts when | ||
// A map[string]string that defines labels to be added to any time series or alerts when |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no strong opinion but I was thinking maybe about something like this to somehow keep track of the original short formatting.
// Defines labels to be added to any time series or alerts when communicating with external systems.
// A map[string]string; by default, no labels are added.
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we agreed on this with @eromanova97
but I have no preference.
I'd keep the systems such as federation, remote storage, and Alertmanager
part though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinions either 👍 It can stay as is, but if we want to keep that consistency, then I would apply the second suggestion
@machine424: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
/verified by existing tests |
@machine424: This PR has been marked as verified by In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| additionalAlertmanagerConfigs | [][AdditionalAlertmanagerConfig](#additionalalertmanagerconfig) | Configures additional Alertmanager instances that receive alerts from the Prometheus component. By default, no additional Alertmanager instances are configured. | | ||
| enforcedBodySizeLimit | string | Enforces a body size limit for Prometheus scraped metrics. If a scraped target's body response is larger than the limit, the scrape will fail. The following values are valid: an empty value to specify no limit, a numeric value in Prometheus size format (such as `64MB`), or the string `automatic`, which indicates that the limit will be automatically calculated based on cluster capacity. The default value is empty, which indicates no limit. | | ||
| externalLabels | ExternalLabels | Defines labels to be added to any time series or alerts when communicating with external systems such as federation, remote storage, and Alertmanager. By default, no labels are added. | | ||
| externalLabels | ExternalLabels | Defines labels to be added to any time series or alerts when communicating with external systems such as federation, remote storage, and Alertmanager. A map[string]string. By default, no labels are added. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| externalLabels | ExternalLabels | Defines labels to be added to any time series or alerts when communicating with external systems such as federation, remote storage, and Alertmanager. A map[string]string. By default, no labels are added. | | |
| externalLabels | ExternalLabels | Defines labels to be added to any time series or alerts when communicating with external systems such as federation, remote storage, and Alertmanager. The type is map[string]string. By default, no labels are added. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @danielmellado @eromanova97 wdty?
A Pandora box was opened (context #2691 (comment)).
// communicating with external systems such as federation, remote storage, | ||
// and Alertmanager. | ||
// By default, no labels are added. | ||
// A map[string]string. By default, no labels are added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// A map[string]string. By default, no labels are added. | |
// A map[string]string; by default, no labels are added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @danielmellado @eromanova97 @juzhao wdty?
A Pandora box was opened (context #2691 (comment)).
I'll probably just close this PR, and let you @eromanova97 decide and open another one :) |
or we can agree on the final wording so I don't have to change it three times. |
agree, I think we could let @eromanova97 decide on the format |
cc @eromanova97
backporting this to release-4.20 in this repo will take days, I don't think that's worth the effort/time.
But you can have this applied to the 4.20>= docs