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

Introducing the Forbidden API maven checks for Hibernate ORM #15729

Merged
merged 3 commits into from
Mar 15, 2021

Conversation

Sanne
Copy link
Member

@Sanne Sanne commented Mar 15, 2021

Fixes #15727

@Sanne Sanne requested a review from yrodiere March 15, 2021 12:31
@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/hibernate-orm Hibernate ORM area/persistence labels Mar 15, 2021
@famod
Copy link
Member

famod commented Mar 15, 2021

Is this something could be moved up to the build-parent later (if needed in another module)?
I'm asking because now the version is in build-parent but the plugin itself is not managed there.
As long as it is used only for the Hibernate extension, the version could also be defined there. Dependabot should still pick it up and the incremental build would be faster.
My 2 cents...YMMV!

@Sanne
Copy link
Member Author

Sanne commented Mar 15, 2021

hi @famod , yes that's my hope but I expect other extensions to pick it up gradually and in small steps - and probably will want to start with a smaller set of checks.

I've started a related conversation on the mailing list.

@gsmet
Copy link
Member

gsmet commented Mar 15, 2021

Sorry CI is all saturated, I had to kill this one for now.

@aloubyansky
Copy link
Member

Shouldn't this also be excluded in quickly?

@Sanne
Copy link
Member Author

Sanne commented Mar 15, 2021

@aloubyansky good idea, thanks! I'll improve that.

@Sanne
Copy link
Member Author

Sanne commented Mar 15, 2021

@aloubyansky fixed that - and rebased. Thanks!

N.B. could someone else approve as well? I'm afraid Yoann's approval isn't authoritative

@famod
Copy link
Member

famod commented Mar 15, 2021

@Sanne Since I have never used this interesting plugin, please allow me this possibly stupid question:
So, say you are working with JDK 11, you add a call that is perfectly fine in 11, but was deprected in let's say 13, you will get a violation message from this plugin because you configured jdk-deprecated-13?
Or is it not working this way?

@Sanne
Copy link
Member Author

Sanne commented Mar 15, 2021

@famod yes that's how it works.
All rules are configurable though, it just so happens to have deprecation rules for all current JDKs included - and that's quite useful.

We also often use different rules for the main vs test code; so for example I could be very picky about maintaining JDK13 compatibility for the "main" code, but relax the rules for testing. E.g. we allow System.out.println in tests, but not in the main code.

@famod
Copy link
Member

famod commented Mar 15, 2021

@Sanne thanks for this explanation.

I see a problem with this though: More often than not, one old method is deprecated and a new better/fixed variant of it is added.
But this variant is not available in 11 or even 8, so you cannot use it. This is kind of a deadlock, no?

@Sanne
Copy link
Member Author

Sanne commented Mar 15, 2021

@famod yes occasionally that's a problem - but I'm not blaming this tool for such issues, it merely helps to ensure the code conforms to our own expectations: when the expectations can't be met, at least I'm aware and we can figure out something; worst case one allows a specific exception to the rules.

And also this is no replacement to actually test with the target JDKs - but it greatly helps to anticipate issues early in the build cycle, and it helps preparing for compatibility with some new JDK w/o necessarily implementing it all in one go.

Let's not focus too much on JDKs too; my favourite use of this tool is to extend the rules with custom restrictions.

@Sanne Sanne added this to the 1.13 - main milestone Mar 15, 2021
@Sanne Sanne merged commit 879c623 into quarkusio:main Mar 15, 2021
@Sanne Sanne deleted the ForbiddenAPI branch March 15, 2021 21:20
@Sanne
Copy link
Member Author

Sanne commented Mar 15, 2021

thanks for the approval help @aloubyansky :)

@famod
Copy link
Member

famod commented Mar 15, 2021

@Sanne Ok, thanks again. I was trying to get a feeling for the limits of this. I also think this can be a nice dev support.
I would have approved in a second. 😉

Btw, GH actions are having a bad evening: https://www.githubstatus.com/incidents/s654n76c1bwr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/hibernate-orm Hibernate ORM area/persistence
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable the Forbidden API plugin
5 participants