Add `PodDisruptionBudget` to helm chart #517

frauniki · 2025-03-13T08:43:21Z

fixed Add PodDisruptionBudget to helm chart #516
Add PodDisruptionBudget to helm chart

cert-manager-prow · 2025-03-13T08:43:34Z

Hi @frauniki. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

frauniki · 2025-09-03T05:35:07Z

@erikgb @wallrj @SgtCoDFish
Hello, could you please review this pull request?
Or, if you know who would be appropriate to ask for a review, could you let me know?

erikgb · 2025-09-03T06:42:46Z

/ok-to-test

…nhance application availability during maintenance feat(values.schema.json): extend schema to include podDisruptionBudget properties for better validation chore(values.yaml): add default podDisruptionBudget configuration with examples for user guidance Signed-off-by: frauniki <[email protected]>

…ion to provide users with guidance on setting disruption budgets Signed-off-by: frauniki <[email protected]>

hawksight · 2025-09-03T06:50:37Z

/cc @hawksight

Signed-off-by: frauniki <[email protected]>

hawksight

Hey @frauniki, thank you for noticing the pdb isn't present in the chart. Really appreciate the PR to get this in place.

I would however like to consider if you could make the values and configuration match the pattern used by cert-manager and trust-manager, so that we have a consistent configuration approach where possible.

Basically, this would mean:

adding an enabled key defaulting to false - consistent with your default
having config keys for both minAvailable & maxUnavailable as doscumented propertirs.
Copy the login in the other charts to prevent a user configuring both and getting a k8s error further down the stack, eg: https://github.com/cert-manager/cert-manager/blob/master/deploy/charts/cert-manager/templates/poddisruptionbudget.yaml#L20-L28

Let me know if that makes sense or you have other reasons to keep it as it, I think it's a trade off but I'd go towards consistency where it makes sense.

…ager pattern Signed-off-by: frauniki <[email protected]>

frauniki · 2025-09-04T04:10:58Z

@hawksight
I fixed the error that was occurring in CI.
Could you please run the test CI again?

hawksight · 2025-09-04T06:18:41Z

/retest

deploy/charts/istio-csr/values.yaml

- Set default - Leave unset (documented only) - Regenerate README and values schema to match This keeps templates simple (with-blocks) and lints clean. Signed-off-by: frauniki <[email protected]>

hawksight · 2025-09-05T08:17:13Z

/lgtm

I think in the current state it's safe to merge. People have to opt into this, at which point they should make their own choice about what setting is best for their installation.

Thank you @ajvn for the lengthy input on this topic too for getting a good default.

At this point we diverge from cert-manager docs, but I think we can revisit that if important later on.

cert-manager-prow · 2025-09-05T08:17:18Z

@hawksight: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

/lgtm

I think in the current state it's safe to merge. People have to opt into this, at which point they should make their own choice about what setting is best for their installation.

Thank you @ajvn for the lengthy input on this topic too for getting a good default.

At this point we diverge from cert-manager docs, but I think we can revisit that if important later on.

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.

hawksight

I can't approve, so back to maybe @erikgb?

erikgb · 2025-09-05T10:58:54Z

I will review this later today or tomorrow. The generated Helm docs can be improved, I think, but I need to find out exactly how. Maybe @frauniki can take a look at https://github.com/cert-manager/helm-tool?

erikgb

I just added some suggestions that I hope will fix the Helm docs and schema.

deploy/charts/istio-csr/values.yaml

Signed-off-by: frauniki <[email protected]>

erikgb

Thanks a lot, @frauniki. Especially for the last-minute adjustments. 🚀

/label tide/merge-method-squash
/lgtm
/approve

cert-manager-prow · 2025-09-05T13:15:22Z

@erikgb: The label(s) /label tide/merge-method-squash cannot be applied. These labels are supported: ``. Is this label configured under labels -> additional_labels or `labels -> restricted_labels` in `plugin.yaml`?

In response to this:

Thanks a lot, @frauniki. Especially for the last-minute adjustments. 🚀

/label tide/merge-method-squash
/lgtm
/approve

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.

cert-manager-prow · 2025-09-05T13:15:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erikgb, hawksight

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:

~~OWNERS~~ [erikgb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

erikgb · 2025-09-05T13:34:32Z

/label tide/squash

cert-manager-prow · 2025-09-05T13:34:35Z

@erikgb: The label(s) /label tide/squash cannot be applied. These labels are supported: ``. Is this label configured under labels -> additional_labels or `labels -> restricted_labels` in `plugin.yaml`?

In response to this:

/label tide/squash

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.

cert-manager-prow bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 13, 2025

cert-manager-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 13, 2025

frauniki force-pushed the add-pdb branch from 8a87fd6 to 746835a Compare March 13, 2025 08:46

cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Mar 13, 2025

cert-manager-prow bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 3, 2025

frauniki added 2 commits September 3, 2025 15:49

docs(istio-csr): add documentation for podDisruptionBudget configurat…

c46d6b4

…ion to provide users with guidance on setting disruption budgets Signed-off-by: frauniki <[email protected]>

cert-manager-prow bot requested a review from hawksight September 3, 2025 06:50

run make generate

35d85d7

Signed-off-by: frauniki <[email protected]>

frauniki force-pushed the add-pdb branch from 746835a to 35d85d7 Compare September 3, 2025 06:59

hawksight suggested changes Sep 3, 2025

View reviewed changes

frauniki force-pushed the add-pdb branch from fdf927f to b5b25be Compare September 3, 2025 18:11

frauniki requested a review from hawksight September 3, 2025 18:11

refactor(helm): align PodDisruptionBudget configuration with cert-man…

2d79d8c

…ager pattern Signed-off-by: frauniki <[email protected]>

frauniki force-pushed the add-pdb branch from b5b25be to 2d79d8c Compare September 4, 2025 04:08

erikgb reviewed Sep 4, 2025

View reviewed changes

deploy/charts/istio-csr/values.yaml Outdated Show resolved Hide resolved

erikgb reviewed Sep 4, 2025

View reviewed changes

deploy/charts/istio-csr/values.yaml Outdated Show resolved Hide resolved

deploy/charts/istio-csr/values.yaml Outdated Show resolved Hide resolved

fix: pdb default values

3c92c35

- Set default - Leave unset (documented only) - Regenerate README and values schema to match This keeps templates simple (with-blocks) and lints clean. Signed-off-by: frauniki <[email protected]>

frauniki requested review from ajvn and erikgb September 5, 2025 07:16

hawksight approved these changes Sep 5, 2025

View reviewed changes

erikgb requested changes Sep 5, 2025

View reviewed changes

deploy/charts/istio-csr/values.yaml Outdated Show resolved Hide resolved

deploy/charts/istio-csr/values.yaml Outdated Show resolved Hide resolved

chart(istio-csr): update values and README (PDB settings)

6693d29

Signed-off-by: frauniki <[email protected]>

frauniki requested a review from erikgb September 5, 2025 13:04

erikgb approved these changes Sep 5, 2025

View reviewed changes

cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2025

cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2025

cert-manager-prow bot merged commit 31882da into cert-manager:main Sep 5, 2025
14 checks passed

erikgb mentioned this pull request Sep 5, 2025

Add support for adding merge method label to PRs cert-manager/testing#1107

Merged

Add PodDisruptionBudget to helm chart #517

Add PodDisruptionBudget to helm chart #517

Uh oh!

Conversation

frauniki commented Mar 13, 2025

Uh oh!

cert-manager-prow bot commented Mar 13, 2025

Uh oh!

frauniki commented Sep 3, 2025

Uh oh!

erikgb commented Sep 3, 2025

Uh oh!

hawksight commented Sep 3, 2025

Uh oh!

hawksight left a comment

Choose a reason for hiding this comment

Uh oh!

frauniki commented Sep 4, 2025

Uh oh!

hawksight commented Sep 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawksight commented Sep 5, 2025

Uh oh!

cert-manager-prow bot commented Sep 5, 2025

Uh oh!

hawksight left a comment

Choose a reason for hiding this comment

Uh oh!

erikgb commented Sep 5, 2025

Uh oh!

erikgb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erikgb left a comment

Choose a reason for hiding this comment

Uh oh!

cert-manager-prow bot commented Sep 5, 2025

Uh oh!

cert-manager-prow bot commented Sep 5, 2025

Uh oh!

Uh oh!

erikgb commented Sep 5, 2025

Uh oh!

cert-manager-prow bot commented Sep 5, 2025

Uh oh!

Uh oh!

Add `PodDisruptionBudget` to helm chart #517

Add `PodDisruptionBudget` to helm chart #517