Skip to content

Feat: Add validations for PagerDuty Receiver in Alertmanager Config CR#8113

Merged
simonpasquier merged 20 commits intoprometheus-operator:mainfrom
nutmos:feat/add-pagerdutyconfig-url-type
Jan 6, 2026
Merged

Feat: Add validations for PagerDuty Receiver in Alertmanager Config CR#8113
simonpasquier merged 20 commits intoprometheus-operator:mainfrom
nutmos:feat/add-pagerdutyconfig-url-type

Conversation

@nutmos
Copy link
Contributor

@nutmos nutmos commented Nov 23, 2025

Description

This PR adds validations to PagerDuty config CR by

  1. Change type in CR from string to URL for url, clientURL, href in pagerDutyImageConfig and href in pagerDutyLinkConfig
  2. Add semantic validation for all URL variables
  3. Remove operator check for URL as it is redundant
  4. Add MinLength=1 validation for the string parameters
  5. Change from string to *string for optional string parameters

Type of change

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Verification

Unit Testing

Changelog entry

- Add URL and string validations for PagerDuty config

@nutmos nutmos requested a review from a team as a code owner November 23, 2025 13:54
// url defines the URL to send requests to.
// +optional
URL string `json:"url,omitempty"`
URL URL `json:"url,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make it a pointer to comply with the API conventions? Same remark for the ClientURL field and the v1beta1 package.

Suggested change
URL URL `json:"url,omitempty"`
URL *URL `json:"url,omitempty"`

@nutmos nutmos marked this pull request as draft November 25, 2025 00:14
@nutmos nutmos marked this pull request as ready for review November 25, 2025 12:25
@nutmos nutmos requested a review from simonpasquier November 25, 2025 12:25
@nutmos
Copy link
Contributor Author

nutmos commented Nov 25, 2025

@simonpasquier Updated, please review

}
}

if config.URL != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we keep the URL validation + add the same for clientURL? The rationale is that the validation webhook might not be deployed and the API validation might not be covering 100% of the edge cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonpasquier I can keep but I'm a bit confused about the webhook part. I don't think the previous code calls admission webhook. L#1100 just refer to the validationv1alpha1 package only.

Correct me if I'm wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry my bad, I agre that we can indeed remove the check here because it's now covered in validationv1alpha1.ValidateAlertmanagerConfig() which shoud be called when the controller selects AlertmanagerConfig resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonpasquier Updated already. Pls review.

@nutmos nutmos requested a review from simonpasquier November 27, 2025 00:06
@nutmos
Copy link
Contributor Author

nutmos commented Nov 27, 2025

@simonpasquier Updated, pls review.

@nutmos
Copy link
Contributor Author

nutmos commented Dec 12, 2025

@simonpasquier Can help review pls?

@nutmos
Copy link
Contributor Author

nutmos commented Dec 18, 2025

@simonpasquier Updated as you suggested. Pls review

@nutmos
Copy link
Contributor Author

nutmos commented Dec 28, 2025

Move to draft and wait for #8211 to merge first.

@nutmos nutmos marked this pull request as draft December 28, 2025 13:13
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 31, 2025
@nutmos nutmos changed the title Feat: Add URL validation for PagerDuty config Feat: Add validations for PagerDuty config Dec 31, 2025
nutmos added 2 commits January 6, 2026 15:27
to fix merge conflicts
to use the new method validatURLPtr
@nutmos nutmos marked this pull request as ready for review January 6, 2026 07:29
@nutmos
Copy link
Contributor Author

nutmos commented Jan 6, 2026

@simonpasquier Could you please review this PR?

@nutmos nutmos changed the title Feat: Add validations for PagerDuty config Feat: Add validations for PagerDuty Receiver in Alertmanager Config CR Jan 6, 2026
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

thanks!

@simonpasquier simonpasquier merged commit 0a6303a into prometheus-operator:main Jan 6, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants