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.

Sign up for GitHub

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

Jump to bottom

Issue #201 assemble-maven-repository respect skip from maven-deploy-plugin #204

Merged
merged 1 commit into from Aug 16, 2021
Merged

Issue #201 assemble-maven-repository respect skip from maven-deploy-plugin #204

merged 1 commit into from Aug 16, 2021

Conversation

olamy
Copy link
Contributor

@olamy olamy commented Jul 28, 2021

Signed-off-by: Olivier Lamy olamy@apache.org

This was referenced Jul 28, 2021
Closed
Merged
laeubi
laeubi requested changes Jul 29, 2021
View reviewed changes
Copy link
Member

@laeubi laeubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, this looks fine to me but some adjustments would be good.

  1. I have already prepared an (incomplete) integration test here , maybe you can integrate this into your PR and complete it with the different cases you like to have covered?
  2. I have added some comments regarding code structure and logging

...repository-plugin/src/main/java/org/eclipse/tycho/plugins/p2/repository/MavenP2SiteMojo.java Outdated Show resolved Hide resolved
...repository-plugin/src/main/java/org/eclipse/tycho/plugins/p2/repository/MavenP2SiteMojo.java Outdated Show resolved Hide resolved
...repository-plugin/src/main/java/org/eclipse/tycho/plugins/p2/repository/MavenP2SiteMojo.java Outdated
* @return the Maven plugin defined in <code>${project.build.plugins}</code> or in
* <code>${project.build.pluginManagement}</code>, or <code>null</code> if not defined.
*/
private static Plugin getPlugin(MavenProject p, String pluginId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laeubi wdym with above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this refers to "Maybe this is better placed in a common helper-class" but I don't see this as a blocker just thought it might better be placed in a common place, I'll check if I found a suitable place for this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

org.eclipse.tycho.core.utils.MavenSessionUtils might be a good place so this code could be reused by other mojos as well...

...repository-plugin/src/main/java/org/eclipse/tycho/plugins/p2/repository/MavenP2SiteMojo.java Outdated
* or <code>null</code> if not found.
* @since 2.6
*/
private static String getPluginParameter(MavenProject p, String pluginId, String param)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is better placed in a common helper-class @mickaelistria any idea what one would be suitable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

org.eclipse.tycho.core.utils.MavenSessionUtils might be a good place so this code could be reused by other mojos as well...

.gitignore Show resolved Hide resolved
...repository-plugin/src/main/java/org/eclipse/tycho/plugins/p2/repository/MavenP2SiteMojo.java Outdated Show resolved Hide resolved
@olamy
Copy link
Contributor Author

olamy commented Jul 29, 2021 •
edited

  1. I have already prepared an (incomplete) integration test here , maybe you can integrate this into your PR and complete it with the different cases you like to have covered?

Great I will cherry-pick there.

@laeubi
Copy link
Member

laeubi commented Jul 30, 2021

TargetPlatformLocationsTest.testMavenLocationRepositories test is flaky atm (no idea why), so don'T mind ignoring it for the moment, this is not related to this change.

laeubi
laeubi approved these changes Jul 30, 2021
View reviewed changes
Copy link
Member

@laeubi laeubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Is there anything left from your side before we merge this?

@olamy
Copy link
Contributor Author

olamy commented Aug 2, 2021

@laeubi all looks good to me.

@mickaelistria
Copy link
Contributor

Please allow me some more time to discuss and review before merging as I'm not confident coupling with other properties owned by other mojos and parsing poms instead of processing model is a good idea in general.
I'm on PTO this week and will look at it next week.

@laeubi
Copy link
Member

laeubi commented Aug 2, 2021

parsing poms instead of processing model

can you explain where poms are parsed here?

@laeubi
Copy link
Member

laeubi commented Aug 4, 2021

@mickaelistria finally the build succeeds, so this is "ready to review" if you are. I just wanted to note that as this is specifically enpower maven projects to publish p2-sites in a maven way it makes perfect sense to me to have some specialties.

To clarify this a bit:

  1. This only applies to reactor projects if these are included in the site as part of a build, so site+projects are part of the same reactor build and there are no artifacts
  2. It does not apply to any declared dependencies that are maybe included (as these have to be deployed anyways before they could be used)
  3. Should we anytime later think it would be good to include reactor projects that are never deployed we can easily add a switch for this (or we can add it now if that helps vanish your concerns)
  4. This is made for supporting common practice (not specific to jetty project) as the alternative would be to add some kind of sophisticated filtering options that require to duplicate configuration already there from a users point of view

@olamy
Copy link
Contributor Author

olamy commented Aug 5, 2021

side note such skip already exists for javadoc plugin. apache/maven-javadoc-plugin#36 (https://issues.apache.org/jira/browse/MJAVADOC-613)

@laeubi laeubi mentioned this pull request Aug 13, 2021
Closed
@laeubi
Copy link
Member

laeubi commented Aug 14, 2021

@mickaelistria do you think we can proceed here? Do you need any further information/background?

@mickaelistria
Copy link
Contributor

I'm OK about adding this functionality. However, are all the commits in the PR related? Please make sure only the relevant changes are merged (and squashed).

@olamy
Issue eclipse-tycho#201 assemble-maven-repository respect skip from m…
2a1080a 
…aven-deploy-plugin

Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@olamy olamy force-pushed the assemble-maven-repository-respect-skip-from-maven-deploy-plugin branch from c2beba7 to 2a1080a Compare August 16, 2021 08:43
@olamy
Copy link
Contributor Author

olamy commented Aug 16, 2021

everything is related and squash done.

laeubi
laeubi approved these changes Aug 16, 2021
View reviewed changes
Copy link
Member

@laeubi laeubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'll merge this then once the build has passed.

@laeubi laeubi merged commit 66bcbff into eclipse-tycho:master Aug 16, 2021
@olamy olamy deleted the assemble-maven-repository-respect-skip-from-maven-deploy-plugin branch August 16, 2021 10:15
@mickaelistria
Copy link
Contributor

Please make sure to add a note in the RELEASE_NOTES to describe what such feature can help with.

@laeubi
Copy link
Member

laeubi commented Aug 16, 2021

It was already added to the release-notes and shows up in the documentation

@mickaelistria mickaelistria linked an issue Aug 16, 2021 that may be closed by this pull request
assemble-maven-repository should respect the skip=true of maven-deployer-plugin #201
Closed
@laeubi laeubi added this to the 2.5 milestone Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laeubi laeubi laeubi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.5
Development

Successfully merging this pull request may close these issues.

assemble-maven-repository should respect the skip=true of maven-deployer-plugin
3 participants
@olamy @laeubi @mickaelistria