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.

Sign up for GitHub

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

Jump to bottom

Add support for max_surge #2630

Merged
merged 5 commits into from
Dec 9, 2024
Merged

Add support for max_surge #2630

merged 5 commits into from
Dec 9, 2024

Conversation

sheneska
Copy link
Contributor

Description

PR fixes #2609

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

Release Note

Release note for CHANGELOG:

...

References

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@sheneska sheneska requested a review from a team as a code owner November 21, 2024 16:44
arybolovlev
arybolovlev reviewed Dec 3, 2024
View reviewed changes
Copy link
Contributor

@arybolovlev arybolovlev left a comment

Choose a reason for hiding this comment

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

All in all, looks good. I have left a few comments. Feel free to ping me if you have any questions.

kubernetes/resource_kubernetes_daemon_set_v1.go Outdated Show resolved Hide resolved
kubernetes/resource_kubernetes_daemon_set_v1.go Outdated Show resolved Hide resolved
kubernetes/resource_kubernetes_daemon_set_v1_test.go Outdated Show resolved Hide resolved
kubernetes/structures_daemonset.go Show resolved Hide resolved
@github-actions github-actions bot added size/S and removed size/XS labels Dec 3, 2024
@github-actions github-actions bot added size/L and removed size/S labels Dec 6, 2024
arybolovlev
arybolovlev approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@arybolovlev arybolovlev left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@sheneska sheneska merged commit 384d2da into main Dec 9, 2024
23 of 26 checks passed
@sheneska sheneska deleted the fix-max-surge branch December 9, 2024 15:14
therc
therc reviewed Dec 17, 2024
View reviewed changes
kubernetes/resource_kubernetes_daemon_set_v1.go
Optional: true,
Default: 1,
ValidateFunc: validation.StringMatch(regexp.MustCompile(`^([1-9][0-9]*|[1-9][0-9]%|[1-9]%|100%)$`), ""),
ValidateFunc: validation.StringMatch(regexp.MustCompile(`^(0|[1-9][0-9]*|[1-9][0-9]%|100%)$`), ""),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this crashes and burns if you set anything from "1%" to "9%". At least it fails on 1% for us:

│ Error: expected value of spec.0.strategy.0.rolling_update.0.max_unavailable to match regular expression "^(0|[1-9][0-9]*|[1-9][0-9]%|100%)$", got 1%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@therc therc therc left review comments

@arybolovlev arybolovlev arybolovlev approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sheneska @arybolovlev @therc