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

Make it possible to disable rewriting log4j.properties #220

Merged
merged 4 commits into from
Jul 5, 2021

Conversation

vierbergenlars
Copy link
Member

Fix #201

@vierbergenlars vierbergenlars added enhancement New feature or request has-jira-ticket Corresponding ticket has been opened in the Xenit Jira for planning labels Jun 22, 2021
@vierbergenlars vierbergenlars added this to the 5.3.1 milestone Jun 22, 2021
Previously, the prefixLog4j task output was inserted directly after the main war, and before any amps/jars.
However, since these applyAmp/applySM/applyDE tasks operate on the stripped copy of the main jar (which includes log4j.properties), these
would overwrite the changes made to log4j.properties by prefixLog4j
@sonarqubecloud
Copy link

@vierbergenlars vierbergenlars requested review from a team and thijslemmens and removed request for a team June 25, 2021 16:40
@vierbergenlars vierbergenlars marked this pull request as ready for review June 25, 2021 16:41
@@ -24,7 +25,10 @@

protected AbstractWarEnrichmentTask() {
outputWar.set(inputWar.flatMap(_x -> getProject().getLayout().getBuildDirectory()
.file("xenit-gradle-plugins/" + getName() + "/" + getName() + ".war")));
.file("xenit-gradle-plugins/" + getName() + "/" + getName() + ".war"))
.map(outputFile -> isEnabled() ? outputFile
Copy link
Contributor

Choose a reason for hiding this comment

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

SInce I'm unfamiliar with the SDK, I am probably missing something, but won't this change result in always throwing when on version < 6.2, while the intention is to only warn when you want to disable the task?

Copy link
Member Author

Choose a reason for hiding this comment

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

What we do here is setting the outputWar property to null when this task is disabled.

This signals to the tasks that consume the output war (so createDockerFile & alfrescoWar) that there is no file to add.

We do not want to warn when disabling the task on < 6.2, we want to actively fail the build with a clear error message.

Returning null from a map() results in a vague exception in Gradle versions before 6.2. The x ? y : z operator is equivalent to if(x) { return y; } else { return z; }, the non-matching side will not be evaluated at all.

On the other hand, keeping the output file when the task is disabled will result in an exception from the tasks that consume the output war, because the promised output file does not actually exists (not generated, because the task is not run).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the thorough explanation.

@@ -24,7 +25,10 @@

protected AbstractWarEnrichmentTask() {
outputWar.set(inputWar.flatMap(_x -> getProject().getLayout().getBuildDirectory()
.file("xenit-gradle-plugins/" + getName() + "/" + getName() + ".war")));
.file("xenit-gradle-plugins/" + getName() + "/" + getName() + ".war"))
.map(outputFile -> isEnabled() ? outputFile
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the thorough explanation.

@vierbergenlars vierbergenlars merged commit f493862 into master Jul 5, 2021
@vierbergenlars vierbergenlars deleted the disable-log4j-modify branch July 5, 2021 13:34
vierbergenlars added a commit that referenced this pull request Oct 22, 2021
Overwriting files in an alfresco WAR with an AMP should work, also when
using the `alfrescoWar` task.
It appears that this issue was already fixed as a side-effect of
f493862 (PR #220), where duplicates strategy was
changed from EXCLUDE to INCLUDE.
vierbergenlars added a commit that referenced this pull request Oct 22, 2021
Overwriting files in an alfresco WAR with an AMP should work, also when
using the `alfrescoWar` task.
It appears that this issue was already fixed as a side-effect of
f493862 (PR #220), where duplicates strategy was
changed from EXCLUDE to INCLUDE.
vierbergenlars added a commit that referenced this pull request Oct 25, 2021
Overwriting files in an alfresco WAR with an AMP should work, also when
using the `alfrescoWar` task.
It appears that this issue was already fixed as a side-effect of
f493862 (PR #220), where duplicates strategy was
changed from EXCLUDE to INCLUDE.
vierbergenlars added a commit that referenced this pull request Oct 25, 2021
Overwriting files in an alfresco WAR with an AMP should work, also when
using the `alfrescoWar` task.
It appears that this issue was already fixed as a side-effect of
f493862 (PR #220), where duplicates strategy was
changed from EXCLUDE to INCLUDE.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request has-jira-ticket Corresponding ticket has been opened in the Xenit Jira for planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to disable modifying log4j.properties
2 participants