add initial support for alertmanager external-url #622
Conversation
|
Just making sure the test fails properly before fixing. |
|
Hey @bwplotka PTAL |
|
cc @bernot-dev as this may be relevant to default-off alertmanager/rule-evaluator |
e2e/alertmanager_test.go
Outdated
| })) | ||
| t.Run("config set", t.subtest(func(ctx context.Context, t *OperatorContext) { | ||
| t.Parallel() | ||
| testAlertmanagerConfig(ctx, t, secret, key) | ||
| })) | ||
| } | ||
|
|
||
| func testAlertmanagerDeployed(ctx context.Context, t *OperatorContext) { | ||
| func testAlertmanagerDeployed(ctx context.Context, t *OperatorContext, spec *monitoringv1.ManagedAlertmanagerSpec) { |
There was a problem hiding this comment.
| func testAlertmanagerDeployed(ctx context.Context, t *OperatorContext, spec *monitoringv1.ManagedAlertmanagerSpec) { | |
| func testAlertmanagerDeployed(ctx context.Context, t *OperatorContext, config *monitoringv1.ManagedAlertmanagerSpec) { |
There was a problem hiding this comment.
Super small nit, but I was quite confused thinking initially this variable as used down there is Alertmanager spec not alertmanager configuration spec (kind of) - a bit more explicitness might help
There was a problem hiding this comment.
Sure no prob. Changed it to config.
e2e/alertmanager_test.go
Outdated
| } | ||
| // We're mainly interested in the dynamic flags but checking the entire set including | ||
| // the static ones is ultimately simpler. | ||
| if externalURL := spec.ExternalURL; externalURL != "" { |
There was a problem hiding this comment.
Bit confused again - can't we prepare wantArgs before looping through Alertmanager statetful set containers? It feel like you do it inside this loop for a reason, but I can't find any 🤔
manifests/setup.yaml
Outdated
| @@ -986,6 +986,9 @@ spec: | |||
| required: | |||
| - key | |||
| x-kubernetes-map-type: atomic | |||
| externalURL: | |||
| type: string | |||
| description: ExternalURL is the external URL the managed Alertmanager will be available under. This is used for generating links back to the Alertmanager itself in fired alerts. | |||
There was a problem hiding this comment.
I wonder if it wouldn't be easier to put exact flag description. It's essentially more than just alert link, it also fixes UI when reverse proxies are used. WDYT?
| description: ExternalURL is the external URL the managed Alertmanager will be available under. This is used for generating links back to the Alertmanager itself in fired alerts. | |
| description: The URL under which Alertmanager is externally reachable (for example, if Alertmanager is served via a reverse proxy). Used for generating relative and absolute links back to Alertmanager itself. If the URL has a path portion, it will be used to prefix all HTTP endpoints served by Alertmanager. If omitted, relevant URL components will be derived automatically. |
e2e/alertmanager_test.go
Outdated
| })) | ||
| t.Run("config set", t.subtest(func(ctx context.Context, t *OperatorContext) { | ||
| t.Parallel() | ||
| testAlertmanagerConfig(ctx, t, secret, key) | ||
| })) | ||
| } | ||
|
|
||
| func testAlertmanagerDeployed(ctx context.Context, t *OperatorContext) { | ||
| func testAlertmanagerDeployed(ctx context.Context, t *OperatorContext, spec *monitoringv1.ManagedAlertmanagerSpec) { |
There was a problem hiding this comment.
Sure no prob. Changed it to config.
e2e/alertmanager_test.go
Outdated
| } | ||
| // We're mainly interested in the dynamic flags but checking the entire set including | ||
| // the static ones is ultimately simpler. | ||
| if externalURL := spec.ExternalURL; externalURL != "" { |
manifests/setup.yaml
Outdated
| @@ -986,6 +986,9 @@ spec: | |||
| required: | |||
| - key | |||
| x-kubernetes-map-type: atomic | |||
| externalURL: | |||
| type: string | |||
| description: ExternalURL is the external URL the managed Alertmanager will be available under. This is used for generating links back to the Alertmanager itself in fired alerts. | |||
Resolves #352