Skip to content
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

Expose the deployment strategy values for the policy controller #749

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

shearn89
Copy link

@shearn89 shearn89 commented May 7, 2024

Description of the change

Prior to this change, the policy controller webhook was not able to have its deployment strategy modified. If you only deployed a single replica, it could not perform a rolling update due to the default maxSurge: 25% being rounded down to 0.

This change exposes those values, so that the maxSurge can be updated and a single instance can be rolled.

Existing or Associated Issue(s)

Fixes #748.

Additional Information

Checklist

  • Chart version bumped in Chart.yaml according to semver. Where applicable, update and bump the versions in any associated umbrella chart
  • Variables are documented in the values.yaml and added to the README.md. The helm-docs utility can be used to generate the necessary content. Use helm-docs --dry-run to preview the content.
  • JSON Schema generated.
  • List tests pass for Chart using the Chart Testing tool and the ct lint command.

Lint output:

$> ct lint --target-branch main --remote upstream
Linting charts...

------------------------------------------------------------------------------------------------------------------------
 Charts to be processed:
------------------------------------------------------------------------------------------------------------------------
 policy-controller => (version: "0.8.0", path: "charts/policy-controller")
------------------------------------------------------------------------------------------------------------------------

"sigstore" already exists with the same configuration, skipping
Linting chart "policy-controller => (version: \"0.8.0\", path: \"charts/policy-controller\")"
Checking chart "policy-controller => (version: \"0.8.0\", path: \"charts/policy-controller\")" for a version bump...
Old chart version: 0.7.0
New chart version: 0.8.0
Chart version ok.
Validating /Users/alex.shearn/opensource/sigstore_helm-charts/charts/policy-controller/Chart.yaml...
Validation success! 👍

Linting chart with values file "charts/policy-controller/ci/ci-values.yaml"...

==> Linting charts/policy-controller
[INFO] Chart.yaml: icon is recommended

1 chart(s) linted, 0 chart(s) failed

------------------------------------------------------------------------------------------------------------------------
 ✔︎ policy-controller => (version: "0.8.0", path: "charts/policy-controller")
------------------------------------------------------------------------------------------------------------------------
All charts linted successfully

@shearn89 shearn89 force-pushed the feature/support-deployment-values-for-policy-controlle-webhook branch 2 times, most recently from 30f3e6e to 711f9e6 Compare May 8, 2024 14:25
@cpanato
Copy link
Member

cpanato commented Jun 27, 2024

can you rebase and sign the dco?

@vipulagarwal
Copy link
Contributor

@shearn89 thanks for adding this feature. Could you do the necessary to get this merged please?

@shearn89
Copy link
Author

shearn89 commented Oct 9, 2024

Sorry, it's been busy at work and home. I'll try to get this ready for merging soon.

Prior to this change, the policy controller webhook was not able to have
its deployment strategy modified. If you only deployed a single replica,
it could not perform a rolling update due to the default `maxSurge:
25%` being rounded down to 0.

This change exposes those values, so that the `maxSurge` can be updated
and a single instance can be rolled.

Fixes sigstore#748.

Signed-off-by: Alex Shearn <[email protected]>
@shearn89 shearn89 force-pushed the feature/support-deployment-values-for-policy-controlle-webhook branch from 711f9e6 to 8e8d223 Compare October 10, 2024 07:49
Signed-off-by: Alex Shearn <[email protected]>
@shearn89
Copy link
Author

shearn89 commented Oct 10, 2024

Okay - rebased, updated, linted, schema generated, and commits signed off!

If you'd like me to squash the commits or squash-merge, let me know. 😁

{{- if .Values.deployment.strategy }}
strategy:
{{ toYaml .Values.deployment.strategy | trim | indent 4 }}
{{ if eq .Values.deployment.strategy.type "Recreate" }}rollingUpdate: null{{ end }}
Copy link
Member

Choose a reason for hiding this comment

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

let's default for the default value when you not set this field, and drop the {{- if .Values.deployment.strategy }} i dont think we need to add any extra complexity for this

Copy link
Author

Choose a reason for hiding this comment

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

Okay - think I've done that although my templating knowledge is not amazing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Policy Controller does not support deployment strategy values
3 participants