jfrog / artifactory-maven-plugin

A Maven plugin to resolve artifacts from Artifactory, deploy artifacts to Artifactory, capture and publish build info.
https://www.jfrog.com/confluence/display/JFROG/Maven+Artifactory+Plugin
Apache License 2.0
24 stars 26 forks source link

Deploy with maven-deploy-plugin if we skip deployment with artifactory-maven-plugin #42

Closed NewLunarFire closed 1 year ago

NewLunarFire commented 1 year ago

Related to: https://github.com/jfrog/artifactory-maven-plugin/issues/38 and also https://github.com/jfrog/artifactory-maven-plugin/issues/32

There are a number of issues with this plugin. 2 of those issues are making me suggest this PR here:

  1. The plugin doesn't follow maven conventions for its artifact deployer. It ignores configuration in the distributionManagement section and in the maven settings.xml file and forces the user to manually specify this information again in the artifactory-maven-plugin configuration.
  2. Following the updates to maven-install-plugin 3.0.0, artifactory-maven-plugin no longer deploys POM for modules that generate a JAR artifact. It seems to have other problems for example with OSGi bundles. The person who created the issue decided to roll back the maven-install-plugin version. but this is not an acceptable compromise for some workflows.

For this reason, we prefer using the standard maven-deploy-plugin for deploying artifacts to our Artifactory instance. We still want to benefit from the build information collection the plugin does, even if not deploying artifacts with the plugin limits the information collected by the modules section of the build information.

We also think that the artifactory-maven-plugin should allow the user to decide how to deploy artifacts:

Currently only options 2 and 3 are possible when using the artifactory-maven-plugin, as it always forces a skip on the maven-deploy-plugin if instanciated.

So to the logic to the change is this:

github-actions[bot] commented 1 year ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

NewLunarFire commented 1 year ago

recheck

NewLunarFire commented 1 year ago

I have read the CLA Document and I hereby sign the CLA

yahavi commented 1 year ago

Thanks for this, @NewLunarFire! Now after https://github.com/jfrog/artifactory-maven-plugin/pull/43 is merged and 3.5.0 is released, you can skip publishing a module's artifacts using <maven.deploy.skip>true</maven.deploy.skip>. However, I'm not sure it would be expected to deploy using the maven-deploy-plugin when a user sets publishArtifacts=false. This would lead to some unexpected scenarios.

Please let me know if you disagree.

NewLunarFire commented 1 year ago

This does not solve the issue I still have with the plugin. It still forces us to either use the broken artifact publisher in this plugin or not publish artifacts at all. I have listed the reasons why using the artifact publisher in this plugin is not a good solution for us. Also, I don't think this plugin should be replacing the maven-deploy-plugin by default.

The artifact publisher still has the problem in issue #38 still exists and rolling back to an earlier version of a built-in plugin in maven should not be the only solution.

Also, as mentionned in issue #32 , the distribution management information is completely ignored using this plugin and we have to specify information in the publisher configuration of the plugin. If the artifact publisher wants to properly replace the one from maven, it should honor this configuration. It also creates issues with secrets management in some cases.

The plugin does a lot of things between the resolver, the build information recorder, and the publisher. Some of those features are strongly coupled together when they don't need to be. The build information recorder is great, but recording this information shouldn't depend on publishing, and publishing artifacts and the build information shouldn't depend on each other either. One can work without the other.

The code changes in the PR are a patch. They're the fix we have done to benefit from the build information recorder without needing to deploy artifacts using the plugin (and therefore keeping out plugins up-to-date and using the configuration we have set in our distribution management). I don't expect the merge to be done as-is, but I would like to see some of the issues I have mentionned addressed, not only those that are listed in the Github Issues but the more fundamental issues I have also commented on.

yahavi commented 1 year ago

Thanks for your feedback, @NewLunarFire. I'm afraid that using the default Maven configuration is beyond the plugin's scope. It would cause some severe issues. For example, we use the same Artifactory credentials for deploying the build-info and the artifacts. As a user, I'd expect the Maven Artifactory plugin to always use the publisher credentials. An example from the other hand - a user may configure his/her credentials may expect the Maven Artifactory plugin to use distribution management server details to publish the build-info. However, nothing promise that the configured server is an Artifactory server. Also, what would you do when there are multiple repositories in the distribution management?

Our custom publisher details are our best way to deploy artifacts and build-info to Artifactory. With that, we could build a publisher optimized for Artifactory and safe enough to use. We do the same thing with our other tools - JFrog CLI, Jenkins Artifactory plugin, JFrog Azure DevOps extension, etc.

Still, we are open to new feature requests and we will keep maintaining this plugin. Here is a fix for issue #38: https://github.com/jfrog/artifactory-maven-plugin/pull/46. Please feel free to let us know if you need anything else.