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 proposal for an optional preflight check in kapp for validating safety of CRD upgrades #729

Merged

Conversation

everettraven
Copy link
Contributor

No description provided.

Copy link

netlify bot commented Feb 27, 2024

Deploy Preview for carvel canceled.

Name Link
🔨 Latest commit 1d187a5
🔍 Latest deploy log https://app.netlify.com/sites/carvel/deploys/65fd435b6755db000893e7b4

@everettraven
Copy link
Contributor Author

Note: I will squash all the commits once this is in an approved state and ready to merge.

@mamachanko
Copy link
Contributor

[thought, non-blocking]: Have you considered use-cases where the CRD author/user would like to control the behaviour from the annotations on the CRD itself, say to bypass them all? Following the precedence of kapp's configuration and annotations I think one could argue for both. Maybe annotations are a potential footgun.

@everettraven
Copy link
Contributor Author

[thought, non-blocking]: Have you considered use-cases where the CRD author/user would like to control the behaviour from the annotations on the CRD itself, say to bypass them all? Following the precedence of kapp's configuration and annotations I think one could argue for both. Maybe annotations are a potential footgun.

@mamachanko Are you referring to a case where a user wants the preflight check enabled, but wants to ignore a specific CRD or which validations to run against a CRD?

@mamachanko
Copy link
Contributor

[thought, non-blocking]: Have you considered use-cases where the CRD author/user would like to control the behaviour from the annotations on the CRD itself, say to bypass them all? Following the precedence of kapp's configuration and annotations I think one could argue for both. Maybe annotations are a potential footgun.

@mamachanko Are you referring to a case where a user wants the preflight check enabled, but wants to ignore a specific CRD or which validations to run against a CRD?

Exactly.

@everettraven
Copy link
Contributor Author

[thought, non-blocking]: Have you considered use-cases where the CRD author/user would like to control the behaviour from the annotations on the CRD itself, say to bypass them all? Following the precedence of kapp's configuration and annotations I think one could argue for both. Maybe annotations are a potential footgun.

@mamachanko Are you referring to a case where a user wants the preflight check enabled, but wants to ignore a specific CRD or which validations to run against a CRD?

Exactly.

So for ignoring a specific CRD, I don't think that is unreasonable but IMO isn't necessary for the base implementation and is something that can be added in the future.

For annotations regarding what validations to run against the CRD, I think that is a bit more nuanced and likely opens up for a wealth of footguns. I think if there is a strong use case for that then I don't see why it couldn't be added, but I think for the initial implementation it makes the most sense to have either all the validations run or none of them (i.e check is enabled/disabled).

@everettraven everettraven force-pushed the proposal/kapp-crd-upgrade-safety branch from 28b0ff0 to 1acb1c6 Compare March 6, 2024 18:23
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.

Thank you so much @everettraven for all that you have been doing to improve kapp ❤️
The proposal looks good to me.

Adding some thoughts as a side note for future: preflight checks might open up another realm (of templating and overlaying) for kapp which might fall outside it's philosophy of deploying and managing applications on kubernetes, so we need to be cautious about what we choose to configure using preflight checks.

@tmshort
Copy link
Contributor

tmshort commented Mar 20, 2024

Is this ready to merge?

@everettraven
Copy link
Contributor Author

Is this ready to merge?

I believe this has now passed the lazy consensus period. I think it is good to merge.

cc @praveenrewar

@joaopapereira
Copy link
Member

Please change the status to Approved in a separate commit and we will be able to merge it

@everettraven
Copy link
Contributor Author

@joaopapereira should be done now in 1d187a5

Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

Approved after discussion in Community Meeting and Lazy consensus vote

@joaopapereira joaopapereira merged commit 9e49dcd into carvel-dev:develop Mar 22, 2024
7 checks passed
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.

8 participants