Skip to content

Conversation

@michael-o
Copy link
Member

No description provided.

Copy link
Contributor

@rfscholte rfscholte left a 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.

asfgit pushed a commit that referenced this pull request Dec 29, 2020
@asfgit asfgit force-pushed the MNG-7020 branch 2 times, most recently from bd28681 to 06cfd2f Compare January 1, 2021 19:36
@michael-o
Copy link
Member Author

@rfscholte Since you approved this, can I go on and merge?

@rfscholte
Copy link
Contributor

IIRC this triggered a new issue. I'll remove my approval and need to have another look at it.

@rfscholte rfscholte self-requested a review February 4, 2021 17:38
@michael-o
Copy link
Member Author

Sure...

@gnodet gnodet requested a review from cstamas October 1, 2021 15:54
Copy link
Member

@cstamas cstamas left a 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)

@rfscholte
Copy link
Contributor

@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.

@michael-o
Copy link
Member Author

@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.

@slachiewicz
Copy link
Member

That was MNG-7122 184db56

@michael-o
Copy link
Member Author

@rfscholte Can you reassess please?

Copy link
Contributor

@mthmulders mthmulders left a 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.

@michael-o
Copy link
Member Author

Let me check...

@michael-o
Copy link
Member Author

@mthmulders I have rebased the branch, I guess the ITs should pass. Let's wait.

@asfgit asfgit closed this in 0f8f22e Jul 24, 2022
@asfgit asfgit merged commit 0f8f22e into master Jul 24, 2022
@michael-o michael-o deleted the MNG-7020 branch July 24, 2022 09:01
gnodet added a commit to gnodet/maven that referenced this pull request Oct 27, 2024
@jira-importer
Copy link

Resolve #8019

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.

7 participants