Skip to content

fix: set default CRD check to false #1057

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

Merged
merged 1 commit into from
Mar 22, 2022
Merged

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Mar 21, 2022

Seting default to false, since the latest release changed the behavior (with the fix). So I would rather have it handled this way for now, and creating a small patch release. So there is no confusion if somebody upgrades.

@csviri csviri self-assigned this Mar 21, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@metacosm
Copy link
Collaborator

The problem is that the behavior was actually wrong in the core, which ended up behaving like the default was false but, as far as I'm aware at least, this behaved properly in both the Spring Boot starter and the Quarkus extension… so in this case, this would be a breaking change.

@csviri
Copy link
Collaborator Author

csviri commented Mar 21, 2022

It would not be breaking I think, I mean we can still keep the default true on those extensions. It won't be breaking in the sense that this is less restrictive. Everything will work that worked until now. But in pure java is actually a breaking change, users already complained on our discord channel.

@csviri
Copy link
Collaborator Author

csviri commented Mar 21, 2022

Will created an another PR where we just log that checking CRD, and how to turn it off. That is always useful.

@metacosm
Copy link
Collaborator

It would not be breaking I think, I mean we can still keep the default true on those extensions. It won't be breaking in the sense that this is less restrictive. Everything will work that worked until now. But in pure java is actually a breaking change, users already complained on our discord channel.

Yes but they're actually complaining that a bug was fixed because that was the intended behavior 😉

@csviri
Copy link
Collaborator Author

csviri commented Mar 21, 2022

It would not be breaking I think, I mean we can still keep the default true on those extensions. It won't be breaking in the sense that this is less restrictive. Everything will work that worked until now. But in pure java is actually a breaking change, users already complained on our discord channel.

Yes but they're actually complaining that a bug was fixed because that was the intended behavior wink

Just users did not know that, nowhere documented, and just everybody used the default. So it can be quite annoying. But basically it also should not be default true IMHO. We should aim for production with defaults.

@metacosm
Copy link
Collaborator

It would not be breaking I think, I mean we can still keep the default true on those extensions. It won't be breaking in the sense that this is less restrictive. Everything will work that worked until now. But in pure java is actually a breaking change, users already complained on our discord channel.

Yes but they're actually complaining that a bug was fixed because that was the intended behavior wink

Just users did not know that, nowhere documented, and just everybody used the default. So it can be quite annoying. But basically it also should not be default true IMHO. We should aim for production with defaults.

You could make the same point about the default behavior of watching all namespaces, which is not realistic for many operators…

@csviri
Copy link
Collaborator Author

csviri commented Mar 22, 2022

You could make the same point about the default behavior of watching all namespaces, which is not realistic for many operators…

I don't understand how it is related. Watch all namespaces is absolutely a valid use case, lots of operators are deployed that way. But don't think there is a use case when we should check CRD on prod during startup. That might be on a dev environment. But on prod it should be already well tested.

@csviri csviri merged commit d4df638 into main Mar 22, 2022
@csviri csviri deleted the check-crd-default-to-false branch March 22, 2022 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants