[MGPG-125] Fix "bestPractices" #95
Conversation
As currently due defaulted values it would always fail, unless user explicitly re-configures plugin. --- https://issues.apache.org/jira/browse/MGPG-125
| import org.codehaus.plexus.util.FileUtils | ||
|
|
||
| var buildLog = new File(basedir, "build.log") | ||
| var logContent = FileUtils.fileRead(buildLog) |
There was a problem hiding this comment.
Groovy can do that without plexus utils:
String logContent = new File( basedir, "build.log").text
| // assert that bestPractice+worstPractice => MojoFailure | ||
| if (!logContent.contains("MojoFailureException: Do not store passphrase in any file")) { | ||
| throw new Exception("Maven build did not fail in consequence of bestPractices+worstPractices") | ||
| } |
There was a problem hiding this comment.
Groovy's native assert is quite suitable here:
assert logContent.contains( "..." )
| if (bestPractices) { | ||
| if (isNotBlank(passphrase) || isNotBlank(passphraseServerId)) { | ||
| // Stop propagating worst practices: passphrase MUST NOT be in any file on disk | ||
| throw new MojoFailureException( | ||
| "Do not store passphrase in any file (disk or SCM repository), rely on GnuPG agent or provide passphrase in " | ||
| + passphraseEnvName + " environment variable."); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
As best practices will probably be extended in the future, why not just refactor out a method here?
| if (bestPractices) { | |
| if (isNotBlank(passphrase) || isNotBlank(passphraseServerId)) { | |
| // Stop propagating worst practices: passphrase MUST NOT be in any file on disk | |
| throw new MojoFailureException( | |
| "Do not store passphrase in any file (disk or SCM repository), rely on GnuPG agent or provide passphrase in " | |
| + passphraseEnvName + " environment variable."); | |
| } | |
| } else { | |
| if (bestPractices) { | |
| checkForBestPractices(); | |
| } else { |
| } else { | ||
| if (!isNotBlank(passphraseServerId)) { | ||
| // default it programmatically: this is needed to handle different cases re bestPractices | ||
| passphraseServerId = GPG_PASSPHRASE; | ||
| } |
There was a problem hiding this comment.
I feel this is a default case inside an else-if nesting. However, as the isolated if(bestPractices) improves readability, I'd say: keep it for now.
One solution would be to refactor out a method like setDefaults(), but probably not worth it.
There was a problem hiding this comment.
These fields are deprecated and are meant to be removed.. so IMO that would be overkill. Soon, when "bestPractices" can be true only (removed), all these will be gone.
|
Btw, this whole PR (with all "non groovy-ness") was copy pasted from other ITs... So maybe we need a "sweeping change" instead where we rewrite the ITs in this plugin to "groovy-like" constructs. |
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <version>2.0.2</version> |
There was a problem hiding this comment.
?
we can use @version.maven-compiler-plugin@ properties for plugins versions are defined in parent
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-install-plugin</artifactId> | ||
| <version>2.2</version> |
|
|
||
| println "Checking for existence of $file" | ||
|
|
||
| if (!file.isFile()) { |
|
Updated PR with comments, but now the new IT is considerably slower (?) and also locally did not, but on CI fails (I guess the force sources creation is needed as there are actually no sources...) @slawekjaranowski ? |
do you have any number to compare? |
|
For last build this PR: 2nd build/commit: Hm, minimal... unsure what I "feel" locally. Maybe that these two ITs use new plugins (hence downoads them) while all the rest use old/ancient plugins but some version? |
|
It can be a time needed for downloading new plugins version into tarted/local-repo build with next without |
As currently due defaulted values it would always fail, unless user explicitly re-configures plugin. So, do not set "default value" on mojo level, instead handle it programmatically, when bestPractices is not set. Added ITs for "bestPractices", one for success and one for failure.
https://issues.apache.org/jira/browse/MGPG-125