Skip to content

[MEAR-287] Fixed failure when destination directory exists#17

Merged
elharo merged 7 commits intoapache:masterfrom
zzzLobster:patch-1
Oct 12, 2020
Merged

[MEAR-287] Fixed failure when destination directory exists#17
elharo merged 7 commits intoapache:masterfrom
zzzLobster:patch-1

Conversation

@zzzLobster
Copy link
Copy Markdown
Contributor

@zzzLobster zzzLobster commented Oct 1, 2020

MEAR-287 Fixed failure when destination directory exists

@zzzLobster zzzLobster changed the title [MEAR-285] Fixed failure when destination directory exists [MEAR-287] Fixed failure when destination directory exists Oct 1, 2020
getLog().info( "Copying artifact [" + module + "] to [" + module.getUri() + "] (unpacked)" );
// Make sure that the destination is a directory to avoid plexus nasty stuff :)
if ( !destinationFile.mkdirs() )
if ( !destinationFile.isDirectory() && !destinationFile.mkdirs() )
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs a test. Assuming this is an issue (I can't reproduce) there are other areas that need to be fixed too.

Copy link
Copy Markdown
Contributor

@mabrarov mabrarov Oct 1, 2020

Choose a reason for hiding this comment

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

Test created in zzzLobster#1. That test covers change in line 752 but not in this line. I'll check if I can extend test. Let me know if it is not required.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm... it wasn't so hard to extend the test. Now it covers both changed lines, i.e. it fails if one of changes is missing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have merged the Marat's PR, So the PR currently contains IT for it

mabrarov and others added 2 commits October 2, 2020 07:31
[MEAR-287] - Integration test for MEAR-287 fix
if ( workDirectory.mkdirs() )
if ( workDirectory.isDirectory() || workDirectory.mkdirs() )
{
getLog().debug( "Created a temporary work directory: " + workDirectory.getAbsolutePath() );
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

log message is now incorrect here; probably need a nested if in this case

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we just simplify log message till:

getLog().debug( "Temporary work directory: " + workDirectory.getAbsolutePath() );

?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Logging was reworked as requested in 304c4b3

/**
* Builds WAR and EAR as part of multi-module project twice so that the 2nd build is guaranteed to be performed when
* target directories and files exist.
* @throws Exception in case of an error.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this @throws clause isn't needed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with useless of this part of this JavaDoc but I prefer to keep code consistent - all tests in this file have this @throws JavaDoc

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't need to be consistent with mistakes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, do you find it appropriate if I remove this useless JavaDoc from all tests in this file?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Removed in eeb458b

@mabrarov
Copy link
Copy Markdown
Contributor

mabrarov commented Oct 3, 2020

@elharo and @hboutemy,
I assume MEAR-287 is a major bug making 3.1.0 release of Maven EAR Plugin breaking existing workflows. Could we speedup / force review of this pull request and release Maven EAR Plugin 3.1.1 ASAP?
Thank you for your work and efforts.

# Conflicts:
#	pom.xml
Copy link
Copy Markdown
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

@mabrarov
Copy link
Copy Markdown
Contributor

What's pending / required for merging this pull request?

@elharo
Copy link
Copy Markdown
Contributor

elharo commented Oct 12, 2020

@elharo elharo merged commit 056f75e into apache:master Oct 12, 2020
@jira-importer
Copy link
Copy Markdown

Resolve #452

@jira-importer
Copy link
Copy Markdown

Resolve #456

@jira-importer
Copy link
Copy Markdown

Resolve #452

@jira-importer
Copy link
Copy Markdown

Resolve #456

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