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

[MDEPLOY-193] Deploy At End feature (no extension) #20

Merged
merged 9 commits into from
Jul 3, 2022
Merged

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Oct 7, 2021

This PR makes deployAtEnd work as expected even
if maven-deploy-plugin is not used as extension.

How it works: it uses mojo Context to store "state markers" (and params):

  • presence of marker means project was "processed"
  • value of state marker tells what should be done
  • if needed, other params are stored as well

UTs adjusted to provide plugin context (was null before).


https://issues.apache.org/jira/browse/MDEPLOY-193

This PR makes deployAtEnd work as expected even
if maven-deploy-plugin is not used as extension.

How it works: it uses mojo Context to store "markers" (and params):
* presence of marker means project was "processed"
* value of marker tells what should be done (true = deploy, false = skipped)
* if needed, other params are stored as well

UTs adjusted to provide plugin context (was null before).
@cstamas cstamas self-assigned this Oct 7, 2021
@cstamas cstamas mentioned this pull request Oct 7, 2021
@rfscholte
Copy link
Contributor

I'm pretty sure I wrote ITs for it, so if they keep working we should be safe. For Maven 3 (and likely 4) this would be fine until https://issues.apache.org/jira/browse/MNG-5666 has been implemented.

Comment on lines 183 to 203
if ( altReleaseDeploymentRepository != null )
{
getPluginContext().put(
DEPLOY_ALT_RELEASE_DEPLOYMENT_REPOSITORY,
altReleaseDeploymentRepository
);
}
if ( altSnapshotDeploymentRepository != null )
{
getPluginContext().put(
DEPLOY_ALT_SNAPSHOT_DEPLOYMENT_REPOSITORY,
altSnapshotDeploymentRepository
);
}
if ( altDeploymentRepository != null )
{
getPluginContext().put(
DEPLOY_ALT_DEPLOYMENT_REPOSITORY,
altDeploymentRepository
);
}
Copy link
Member

Choose a reason for hiding this comment

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

If we can add null value - I would not check for null

Copy link
Member

Choose a reason for hiding this comment

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

I still think that checking for null is not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I am joining this question also.

Copy link
Member Author

Choose a reason for hiding this comment

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

w/o nullcheck:

Caused by: java.lang.NullPointerException
    at java.util.concurrent.ConcurrentHashMap.putVal (ConcurrentHashMap.java:1011)
    at java.util.concurrent.ConcurrentHashMap.put (ConcurrentHashMap.java:1006)
    at org.apache.maven.plugins.deploy.DeployMojo.execute (DeployMojo.java:183)
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:137)

You know, that plugin context is not a simple Map...

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

We also need DEPLOY_PROCESSED_MARKER if deployAtEnd if false.
One module can override it.

@cstamas
Copy link
Member Author

cstamas commented Jun 25, 2022

@gnodet @michael-o @slawekjaranowski ping, let's agree all this PR does is:

  • not changing existing behaviour (re last module)
  • but removes the requirement for extension

So IMHO is def an improvement

@michael-o
Copy link
Member

Same here

@michael-o
Copy link
Member

@gnodet @michael-o @slawekjaranowski ping, let's agree all this PR does is:

* not changing existing behaviour (re last module)

* but removes the requirement for extension

So IMHO is def an improvement

I do agree. Minimal scope.

Also, be clearer re intent. Still, cannot use enum directly, as
due classloading, there is no single enum.
@cstamas
Copy link
Member Author

cstamas commented Jun 29, 2022

We also need DEPLOY_PROCESSED_MARKER if deployAtEnd if false. One module can override it.

Done

@cstamas cstamas requested a review from hboutemy July 2, 2022 20:03
@cstamas cstamas merged commit 89c28fb into master Jul 3, 2022
@cstamas cstamas deleted the deploy-at-end branch July 3, 2022 18:18
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.

5 participants