Split precommit plugins into internal and external #65102
Conversation
breskeby
added
:Delivery/Build
breskeby
force-pushed
the
seperate-internal-precommit
branch
from
81198e1 to
670ee71
Compare
There was a problem hiding this comment.
removing JarHellPrecommitPlugin and ThirdPartyAuditPrecommitPlugin here
There was a problem hiding this comment.
We only need this for internal builds
There was a problem hiding this comment.
the actual change in this class except porting it to java
There was a problem hiding this comment.
Since we removed jarhell and thirdpartyaudit from InternalPrecommitTasks what then applies those? Shouldn't we always apply PrecommitTasks, not just as part of this else condition?
There was a problem hiding this comment.
PrecommitTasks are applied within InternalPrecommitTasks
There was a problem hiding this comment.
address package change
There was a problem hiding this comment.
we only wanna test the testing convention plugin here
|
I made a general comment related to this on #63697 (comment) regarding backwards compatibility and deprecation cycles. This PR as is is a breaking change for likely most external plugin authors out there and we might want to reconsider how to approach this and be a bit more conservative in introducing this change. |
breskeby
requested review from
jakelandis and
mark-vieira
and removed request for
jakelandis
|
Pinging @elastic/es-delivery (Team:Delivery) |
There was a problem hiding this comment.
Looks like we migrated a few Groovy files to Java but didn't move them to the java source set. We ideally don't want to rely on joint compilations sicne it's incredibly slow. If moving to java isn't possible because of all the dependencies that'd have to move over too that's fine, let's just keep as Groovy for now rather than rely on Groovy joint-compilation.
There was a problem hiding this comment.
Shouldn't this be true?
There was a problem hiding this comment.
I would comment above but GitHub sucks. We should similarly wrap all the isModule stuff above in this check. For external projects it will always be a plugin, not a module, so we shouldn't accidentally treat it as such if an external author incidentally uses conflicting project naming conventions.
There was a problem hiding this comment.
I'd prefer we just stick with the slightly more verbose streams api here than rely on internal Gradle util classes. Or introduce our own util method.
There was a problem hiding this comment.
For now kept this to be groovy so reverted this change
There was a problem hiding this comment.
Since we removed jarhell and thirdpartyaudit from InternalPrecommitTasks what then applies those? Shouldn't we always apply PrecommitTasks, not just as part of this else condition?
There was a problem hiding this comment.
Can we go ahead and port this to Java too?
There was a problem hiding this comment.
unfortunately we can't at the moment. Whats blocking porting most groovy classes here to java is the AntTask class and classes inheriting this one. It makes use of groovys ant builder. I'm sure we can rewrite the LicenseHeadersTask task in a way to not rely on ant under the hood and then move most of the precommit stuff over to plain java. But not as part of this PR
- avoid joint java groovy compilation
breskeby
force-pushed
the
seperate-internal-precommit
branch
from
3421ece to
ece5df5
Compare
* Split precommit plugins into internal and external * Introduce interface for InternalPlugins * avoid joint java groovy compilation * Move isModule handling into internal only
This PR splits the precommit checks into internal and external as we do not want to enforce Elasticsearch build specific internal checks being enforced on external plugin authors.
On the way also ported some of the touched classes from groovy to java and introduced the general notion of an
InternalPlugin.Fixes #63697