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

Add handling of updates to a field's enum values to the CRD Upgrade Safety preflight check #921

Conversation

everettraven
Copy link
Contributor

What this PR does / why we need it:

  • Adds a generic, extensible validator for handling changes to existing fields in an existing version of a CRD schema
  • Adds an initial field change validator for handling changes to an existing fields allowed enum values
    • Rejects when the existing version of the field does not have enum restrictions but the new version adds enum restriction
    • Rejects when enums from the existing version of the field are removed in the new version
    • Approves when enum values are added to the list of allowed enum values

Which issue(s) this PR fixes:

Fixes #916

Does this PR introduce a user-facing change?

Add logic to the CRD Upgrade Safety pre-flight check to only allow additions to the valid enum values of an existing field in an existing version of a CRD schema

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
Copy link
Contributor Author

Note: I'll squash commits after reviews

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.

Looks pretty good apart from some minor comments.

I also wanted to mention one additional thing apart from the comments. In the Carvel world, we have been capitalising error strings (examples can be found across the projects). I know, I know, this is not as per the go style decisions, however since this is being done across the carvel tool suite, the error messages are consistent within Carvel. This might be something worth discussing outside the scope of this PR (maybe in the community meeting?). At this point I don't want to force you to capitalise error strings as even now when we get errors from some other library, they are not capitalised and the error messages are a mix of the two, which would be something similar with preflight checks.

@everettraven everettraven force-pushed the feature/crdus-handle-field-updates branch from b192141 to ce52021 Compare April 15, 2024 14:20
Signed-off-by: everettraven <everettraven@gmail.com>
@everettraven everettraven force-pushed the feature/crdus-handle-field-updates branch from ce52021 to a1da50a Compare April 15, 2024 14:25
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.

Changes look good to me!

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.

Looks good!!

pkg/kapp/crdupgradesafety/change_validator.go Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add validation to CRDUpgradeSafety preflight check to allow adding more enum values to existing field
3 participants