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 CRD Upgrade Safety preflight check #901

Conversation

everettraven
Copy link
Contributor

@everettraven everettraven commented Mar 12, 2024

What this PR does / why we need it:

Adds an initial implementation of carvel-dev/carvel#729

Includes the new CRDUpgradeSafety preflight check implementation and 2 of the known breaking changes during a CRD upgrade:

  • Scope change (i.e cluster <--> namespace)
  • Removal of existing stored version

Implementation is broken down into individual commits for review purposes, I am happy to squash commits before merging:

  • Initial, generic preflight check implementation: c2a34a5
  • Scope change validation: 228ead6
  • Stored version validation: 28d6094

Which issue(s) this PR fixes:

Fixes #909

Does this PR introduce a user-facing change?

Adds an initial `CRDUpgradeSafety` preflight check implementation with checks to ensure a CRD upgrade does not include a scope change and/or removal of existing stored versions

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 marked this pull request as ready for review March 25, 2024 17:58
@everettraven everettraven changed the title WIP: Add CRD Upgrade Safety preflight check Add CRD Upgrade Safety preflight check Mar 25, 2024
@everettraven
Copy link
Contributor Author

Note: I'm planning to squash commits prior to merging, but after reviews have been done to keep it easier to review

@everettraven everettraven force-pushed the feature/crd-upgrade-safety-preflight branch 2 times, most recently from dc3799e to a42be1a Compare March 26, 2024 13:28
@everettraven
Copy link
Contributor Author

Note: I'm planning to squash commits prior to merging, but after reviews have been done to keep it easier to review

Whelp a botched rebase caused some problems that ended up in me deciding a squash after the rebase would be cleaner than the individual commits. Apologies to the reviewers!

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! Thank you so much for making kapp better ❤️
I think a rebase would be required as Todd's PR is merged.

and some initial validations for handling scope changes
and removal of existing stored versions

Signed-off-by: everettraven <everettraven@gmail.com>
@everettraven everettraven force-pushed the feature/crd-upgrade-safety-preflight branch from 6fbc4ef to e12d91d Compare March 28, 2024 15:01
@everettraven
Copy link
Contributor Author

Thanks @praveenrewar ! I've just rebased, updated, and squashed the commits. Should be ready to merge!

The update made was https://github.com/carvel-dev/kapp/pull/901/files#diff-02d1be65fcbbdbd73025e5327506d584006201a288ba1c598b92e7d67467529eR52-R54 to satisfy the change to the preflight.Check interface Todd had made.

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 a basic CRDUpgradeSafety preflight check
2 participants