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

[MNG-7055] Fix G level metadata handling #555

Merged
merged 10 commits into from
Oct 2, 2021
Merged

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Sep 28, 2021

Maven Artifact Transfer silently prevents group level metadata to reach Resolver and causes metadata loss on install/deploy. Fix is to "bridge" this from maven-resolver-provider (and core) by reusing the actual metadata that m-plugin-p:addPluginArtifactMetadata mojo adds, but m-a-t filters out.

https://issues.apache.org/jira/browse/MNG-7055

Just make Resolver handle it. This is TBD, as actual
prefix still needs somehow to get there.
@cstamas cstamas requested a review from michael-o September 28, 2021 14:16
@cstamas cstamas self-assigned this Sep 28, 2021
@mthmulders
Copy link
Contributor

How does this relate to MNG-7055?

@cstamas
Copy link
Member Author

cstamas commented Sep 28, 2021

How does this relate to MNG-7055?

Fixes it (not yet, once done)

@cstamas
Copy link
Member Author

cstamas commented Sep 28, 2021

The ITs reported in MNG-7055 to fail with 3.x of m-install-p and m-deploy-p are passing OK (!), but now another IT fails:

[INFO] 
[INFO] Results:
[INFO] 
Error:  Failures: 
Error:    MavenIT0138PluginLifecycleTest>AbstractMavenIntegrationTestCase.runTest:265->testit0138:60 Expected file was not found: /home/runner/work/maven/maven/maven-integration-testing/core-it-suite/target/test-classes/it0138/target/plugin-add-plugin-artifact-metadata.txt
[INFO] 
Error:  Tests run: 882, Failures: 1, Errors: 0, Skipped: 0

@cstamas
Copy link
Member Author

cstamas commented Sep 28, 2021

Patched ITs with apache/maven-integration-testing#114 all pass OK:

[INFO] Tests run: 879, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 192.019 s - in org.apache.maven.it.IntegrationTestSuite
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 879, Failures: 0, Errors: 0, Skipped: 0

@rfscholte
Copy link
Contributor

A similar change needs to be done for both plugins, in case it is used with Maven 3

@mthmulders
Copy link
Contributor

How does this relate to MNG-7055?

Fixes it (not yet, once done)

I'm a bit sad that the work that @MartinKanters and me had done so far (announced in the JIRA ticket and more recently here on GitHub) is apparently of no use.

@rfscholte
Copy link
Contributor

@mthmulders this is a fix for Maven 4, but not for Maven 3

@mthmulders
Copy link
Contributor

@mthmulders this is a fix for Maven 4, but not for Maven 3

I don't think the fix that @MartinKanters and I have worked on is compatible with this approach.

@cstamas cstamas changed the title Fix G level metadata handling [MNG-7055] Fix G level metadata handling Sep 30, 2021
@cstamas cstamas marked this pull request as ready for review September 30, 2021 12:25
@cstamas cstamas requested review from gnodet and rfscholte September 30, 2021 12:55
@gnodet
Copy link
Contributor

gnodet commented Sep 30, 2021

Does this mean that the AddPluginArtifactMetadataMojo from maven-plugin-plugin is completely useless ? This is actually one of the pain point because it depends on the GroupRepositoryMetadata which is in maven-compat.

@cstamas
Copy link
Member Author

cstamas commented Sep 30, 2021

Does this mean that the AddPluginArtifactMetadataMojo from maven-plugin-plugin is completely useless ? This is actually one of the pain point because it depends on the GroupRepositoryMetadata which is in maven-compat.

Yes and no: Yes, as we could get the only missing piece even without it (the goalPrefix that is plugin configuration). The "price" would be high, go thru plugin config/DOM stuff.... No, as due that above, it made this PR stupid simple (just reuse the data that mojo gave to us).

In general (and that's why I created this PR initially to drop this useless mojo) I'd drop it, but this seems like "simple first step", and later we can change it at will. Given m-plugin-p is "just a plugin" but is still quite often referenced in core (so somewhat not "just a plugin"), I think we could do more here, for example change the mojo to just stick some values somewhere (ie. session) and alter the provider in maven core and voila, GroupRepositoryMetadata is not needed....

Use provider as SISU Providers obey scope.

In UT two affected UTs needs a bit of extra work,
enter scope and fake session using Mockito
@gnodet
Copy link
Contributor

gnodet commented Oct 1, 2021

Related to apache/maven-plugin-tools#37 wrt GroupRepositoryMetadata ...

@cstamas cstamas merged commit d141957 into master Oct 2, 2021
@cstamas cstamas deleted the fix-group-metadata-deploy branch October 2, 2021 13:22
cstamas added a commit that referenced this pull request Mar 13, 2022
Maven Artifact Transfer silently prevents group level metadata to reach Resolver and causes metadata loss on install/deploy.
Fix is to "bridge" this from maven-resolver-provider (and core) by reusing the actual metadata that
m-plugin-p:addPluginArtifactMetadata mojo adds, but m-a-t filters out.

This is backport of commit d141957 to maven-3.9.x branch.
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.

4 participants