-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
[MNG-7020] Remove Maven 2 WagonExcluder backward compat code #394
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on just the code this looks good and based on the comment this should be safe to do. I don't know where to expect any impact, if any.
bd28681
to
06cfd2f
Compare
@rfscholte Since you approved this, can I go on and merge? |
IIRC this triggered a new issue. I'll remove my approval and need to have another look at it. |
Sure... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure what to say here...
This was in place for Maven2 plugins (running in Maven3). Hopefully today they will break with Maven4, so WagonExcluder is really not needed as plugin will not even execute. Still, IMO we'd need some safety "thing" maybe simply reject plugins declaring dependency on Maven artifact(s) having version 2.x? Just to be on safe side? (like prerequisite, but other way around)
@cstamas @michael-o I thought we also worked on that "require maven-plugin-api version", but can't find the ticket. For me that must be implemented first before accepting this PR. |
That is true and that is why I have not yet merged it. |
@rfscholte Can you reassess please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All integration tests are failing - could it be it's an outdated version of the IT's? Other than that, it looks fine to me.
Let me check... |
@mthmulders I have rebased the branch, I guess the ITs should pass. Let's wait. |
No description provided.