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 validation for changes to minimum constraints for CRD fields #944

Merged
merged 2 commits into from
May 2, 2024
Merged

add validation for changes to minimum constraints for CRD fields #944

merged 2 commits into from
May 2, 2024

Conversation

everettraven
Copy link
Contributor

What this PR does / why we need it:

  • Adds validations to ensure breaking changes are not made to a CRD field during an upgrade by adding or increasing the following field constraints:
    • minimum
    • minLength
    • minProperties
    • minItems

Which issue(s) this PR fixes:

Fixes #913

Does this PR introduce a user-facing change?

Add validation to the CRDUpgradeSafety pre-flight check that minimum field constraints are not added or increased during a CRD upgrade

Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:

@everettraven everettraven force-pushed the feature/crdus-minimum-decrease branch from 868776f to 90a766b Compare April 29, 2024 20:09
@everettraven
Copy link
Contributor Author

cc @rashmigottipati

rashmigottipati
rashmigottipati reviewed Apr 30, 2024
View reviewed changes
Copy link
Contributor

@rashmigottipati rashmigottipati 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, overall. Just left a few review comments/nits.

pkg/kapp/crdupgradesafety/change_validator.go Show resolved Hide resolved
pkg/kapp/crdupgradesafety/change_validator.go Show resolved Hide resolved
pkg/kapp/crdupgradesafety/change_validator.go Show resolved Hide resolved
pkg/kapp/crdupgradesafety/change_validator.go Show resolved Hide resolved
pkg/kapp/crdupgradesafety/change_validator.go Show resolved Hide resolved
@everettraven
add godoc comments
4d64695 
Signed-off-by: everettraven <[email protected]>
rashmigottipati
rashmigottipati approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@rashmigottipati rashmigottipati left a comment

Choose a reason for hiding this comment

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

/lgtm

praveenrewar
praveenrewar approved these changes May 2, 2024
View reviewed changes
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

LGTM!

@praveenrewar praveenrewar merged commit 4c75f2e into carvel-dev:develop May 2, 2024
5 checks passed
@rashmigottipati rashmigottipati mentioned this pull request May 8, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rashmigottipati rashmigottipati rashmigottipati approved these changes

@praveenrewar praveenrewar praveenrewar approved these changes

@100mik 100mik Awaiting requested review from 100mik

Assignees
No one assigned
Labels
None yet
Projects
Carvel
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add validation to CRDUpgradeSafety preflight check to allow decreasing minimum values on existing fields
3 participants
@everettraven @rashmigottipati @praveenrewar