Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,36 @@ tests:
spec:
userDefined:
mode: "Disabled"
- name: Should accept userAlertmanagerConfigSelection on alertmanagerConfig
initial: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
userDefined:
mode: "Disabled"
alertmanagerConfig:
deploymentMode: "DefaultConfig"
userAlertmanagerConfigSelection: Selectable
expected: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
userDefined:
mode: "Disabled"
alertmanagerConfig:
deploymentMode: "DefaultConfig"
userAlertmanagerConfigSelection: Selectable
Comment on lines +21 to +39
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add test coverage for the "None" enum value.

The validation error on line 50 indicates that userAlertmanagerConfigSelection supports two values: "Selectable" and "None". Currently only "Selectable" is tested for acceptance. Add a similar test case that verifies "None" is also accepted and preserved.

📋 Proposed additional test case
    - name: Should accept userAlertmanagerConfigSelection None value on alertmanagerConfig
      initial: |
        apiVersion: config.openshift.io/v1alpha1
        kind: ClusterMonitoring
        spec:
          userDefined:
            mode: "Disabled"
          alertmanagerConfig:
            deploymentMode: "DefaultConfig"
            userAlertmanagerConfigSelection: None
      expected: |
        apiVersion: config.openshift.io/v1alpha1
        kind: ClusterMonitoring
        spec:
          userDefined:
            mode: "Disabled"
          alertmanagerConfig:
            deploymentMode: "DefaultConfig"
            userAlertmanagerConfigSelection: None
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml`
around lines 21 - 39, Add a parallel test case for the other enum value by
duplicating the existing test named "Should accept
userAlertmanagerConfigSelection on alertmanagerConfig" and change the
alertmanagerConfig.userAlertmanagerConfigSelection value to "None" in both the
initial and expected YAML blocks; ensure the new test name reflects the change
(e.g., "Should accept userAlertmanagerConfigSelection None value on
alertmanagerConfig") and that the key userAlertmanagerConfigSelection appears
exactly as in the original so the test validates preservation of the "None"
enum.

- name: Should reject invalid userAlertmanagerConfigSelection on alertmanagerConfig
initial: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
userDefined:
mode: "Disabled"
alertmanagerConfig:
deploymentMode: "DefaultConfig"
userAlertmanagerConfigSelection: Enabled
expectedError: 'spec.alertmanagerConfig.userAlertmanagerConfigSelection: Unsupported value: "Enabled": supported values: "Selectable", "None"'
Comment on lines +21 to +50
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add test coverage for optional field behavior.

Per the PR summary and AI-generated description, userAlertmanagerConfigSelection is an optional field. Add a test case that explicitly verifies an alertmanagerConfig object can be created without this field, confirming the optional behavior and default handling.

📋 Proposed additional test case
    - name: Should accept alertmanagerConfig without userAlertmanagerConfigSelection
      initial: |
        apiVersion: config.openshift.io/v1alpha1
        kind: ClusterMonitoring
        spec:
          userDefined:
            mode: "Disabled"
          alertmanagerConfig:
            deploymentMode: "DefaultConfig"
      expected: |
        apiVersion: config.openshift.io/v1alpha1
        kind: ClusterMonitoring
        spec:
          userDefined:
            mode: "Disabled"
          alertmanagerConfig:
            deploymentMode: "DefaultConfig"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml`
around lines 21 - 50, Add a new test case that verifies alertmanagerConfig can
be created without the optional field userAlertmanagerConfigSelection by adding
an entry named "Should accept alertmanagerConfig without
userAlertmanagerConfigSelection" that provides an initial ClusterMonitoring with
spec.alertmanagerConfig containing only deploymentMode: "DefaultConfig" and
expects the same object back; ensure you reference the alertmanagerConfig object
and the userAlertmanagerConfigSelection symbol so the test confirms omission is
accepted and defaults are handled.

- name: Should reject ContainerResource with duplicate names
initial: |
apiVersion: config.openshift.io/v1alpha1
Expand Down
29 changes: 29 additions & 0 deletions config/v1alpha1/types_cluster_monitoring.go
Original file line number Diff line number Diff line change
Expand Up @@ -794,8 +794,37 @@ type AlertmanagerConfig struct {
// When set to CustomConfig, the Alertmanager will be deployed with custom configuration.
// +optional
CustomConfig AlertmanagerCustomConfig `json:"customConfig,omitempty,omitzero"`
// userAlertmanagerConfigSelection is an optional field that controls whether user-defined
// namespaces can be selected for AlertmanagerConfig lookups on the platform Alertmanager
// instance in the `openshift-monitoring` namespace.
// Valid values are Selectable and None.
// When set to Selectable, the platform Alertmanager discovers AlertmanagerConfig resources
// in user-defined namespaces. This is equivalent to `enableUserAlertmanagerConfig: true` in
// the cluster-monitoring-config ConfigMap.
Comment on lines +801 to +803
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How are these resources discovered? Is there any additional configuration options we may want to support for how these are discovered?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The (Prometheus) operator discovers all AlertmanagerConfig resources in all namespaces which are not labelled with openshift.io/cluster-monitoring: true (aka user namespaces).

// When set to None, user-defined namespaces are not selected for AlertmanagerConfig lookups
// on the platform Alertmanager. This is equivalent to `enableUserAlertmanagerConfig: false`
// in the cluster-monitoring-config ConfigMap.
// This setting only applies when the user-workload monitoring Alertmanager is not enabled.
// When omitted, the default value is None.
// +optional
// +kubebuilder:validation:Enum=Selectable;None
UserAlertmanagerConfigSelection UserAlertmanagerConfigSelection `json:"userAlertmanagerConfigSelection,omitempty"`
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should this be moved to AlertmanagerCustomConfig instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems reasonable to me.

}

// UserAlertmanagerConfigSelection controls whether the platform Alertmanager selects
// AlertmanagerConfig resources from user-defined namespaces.
// +enum
type UserAlertmanagerConfigSelection string

const (
// UserAlertmanagerConfigSelectionSelectable enables user-defined namespaces to be selected
// for AlertmanagerConfig lookups on the platform Alertmanager.
UserAlertmanagerConfigSelectionSelectable UserAlertmanagerConfigSelection = "Selectable"
// UserAlertmanagerConfigSelectionNone disables user-defined namespaces from being selected
// for AlertmanagerConfig lookups on the platform Alertmanager.
UserAlertmanagerConfigSelectionNone UserAlertmanagerConfigSelection = "None"
)

// AlertmanagerCustomConfig represents the configuration for a custom Alertmanager deployment.
// alertmanagerCustomConfig provides configuration options for the default Alertmanager instance
// that runs in the `openshift-monitoring` namespace. Use this configuration to control
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -888,6 +888,24 @@ spec:
- DefaultConfig
- CustomConfig
type: string
userAlertmanagerConfigSelection:
description: |-
userAlertmanagerConfigSelection is an optional field that controls whether user-defined
namespaces can be selected for AlertmanagerConfig lookups on the platform Alertmanager
instance in the `openshift-monitoring` namespace.
Valid values are Selectable and None.
When set to Selectable, the platform Alertmanager discovers AlertmanagerConfig resources
in user-defined namespaces. This is equivalent to `enableUserAlertmanagerConfig: true` in
the cluster-monitoring-config ConfigMap.
When set to None, user-defined namespaces are not selected for AlertmanagerConfig lookups
on the platform Alertmanager. This is equivalent to `enableUserAlertmanagerConfig: false`
in the cluster-monitoring-config ConfigMap.
This setting only applies when the user-workload monitoring Alertmanager is not enabled.
When omitted, the default value is None.
enum:
- Selectable
- None
type: string
required:
- deploymentMode
type: object
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -888,6 +888,24 @@ spec:
- DefaultConfig
- CustomConfig
type: string
userAlertmanagerConfigSelection:
description: |-
userAlertmanagerConfigSelection is an optional field that controls whether user-defined
namespaces can be selected for AlertmanagerConfig lookups on the platform Alertmanager
instance in the `openshift-monitoring` namespace.
Valid values are Selectable and None.
When set to Selectable, the platform Alertmanager discovers AlertmanagerConfig resources
in user-defined namespaces. This is equivalent to `enableUserAlertmanagerConfig: true` in
the cluster-monitoring-config ConfigMap.
When set to None, user-defined namespaces are not selected for AlertmanagerConfig lookups
on the platform Alertmanager. This is equivalent to `enableUserAlertmanagerConfig: false`
in the cluster-monitoring-config ConfigMap.
This setting only applies when the user-workload monitoring Alertmanager is not enabled.
When omitted, the default value is None.
enum:
- Selectable
- None
type: string
required:
- deploymentMode
type: object
Expand Down
7 changes: 4 additions & 3 deletions config/v1alpha1/zz_generated.swagger_doc_generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions openapi/generated_openapi/zz_generated.openapi.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading