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

[MGPG-125] Fix "bestPractices" #95

Merged
merged 5 commits into from
Apr 13, 2024
Merged

[MGPG-125] Fix "bestPractices" #95

merged 5 commits into from
Apr 13, 2024

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Apr 11, 2024

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

As currently due defaulted values it would always fail, unless
user explicitly re-configures plugin.

---

https://issues.apache.org/jira/browse/MGPG-125
@cstamas cstamas self-assigned this Apr 11, 2024
Copy link

@bmarwell bmarwell left a comment

Choose a reason for hiding this comment

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

Mostly a little groovy and nesting readability thoughts.

Comment on lines 21 to 24
import org.codehaus.plexus.util.FileUtils

var buildLog = new File(basedir, "build.log")
var logContent = FileUtils.fileRead(buildLog)

Choose a reason for hiding this comment

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

Groovy can do that without plexus utils:

String logContent = new File( basedir, "build.log").text

Comment on lines 26 to 29
// 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")
}

Choose a reason for hiding this comment

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

Groovy's native assert is quite suitable here:

assert logContent.contains( "..." )

Comment on lines 303 to 310
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 {

Choose a reason for hiding this comment

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

As best practices will probably be extended in the future, why not just refactor out a method here?

Suggested change
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 {

Comment on lines +310 to +314
} else {
if (!isNotBlank(passphraseServerId)) {
// default it programmatically: this is needed to handle different cases re bestPractices
passphraseServerId = GPG_PASSPHRASE;
}

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@cstamas cstamas Apr 11, 2024

Choose a reason for hiding this comment

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

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.

@cstamas
Copy link
Member Author

cstamas commented Apr 11, 2024

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>
Copy link
Member

Choose a reason for hiding this comment

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

?
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>
Copy link
Member

Choose a reason for hiding this comment

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

😄


println "Checking for existence of $file"

if (!file.isFile()) {
Copy link
Member

Choose a reason for hiding this comment

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

assert file.isFile()

@cstamas
Copy link
Member Author

cstamas commented Apr 12, 2024

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 ?

@slawekjaranowski
Copy link
Member

but now the new IT is considerably slower ...

do you have any number to compare?

@cstamas
Copy link
Member Author

cstamas commented Apr 12, 2024

For last build this PR:

[INFO] Building: sign-release-best-practices/pom.xml
[INFO] run post-build script verify.groovy
[INFO]           sign-release-best-practices/pom.xml .............. SUCCESS (4.002 s)

2nd build/commit:

[INFO] Building: sign-release-best-practices/pom.xml
[INFO] run post-build script verify.groovy
[INFO]           sign-release-best-practices/pom.xml .............. SUCCESS (3.877 s)

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?

@slawekjaranowski
Copy link
Member

It can be a time needed for downloading new plugins version into tarted/local-repo
Too small difference to more investigating

build with mvn clean ...

gpg
[INFO]           sign-release-best-practices/pom.xml .............. SUCCESS (8.135 s)

bc
[INFO]           sign-release-best-practices/pom.xml .............. SUCCESS (2.189 s)

next without clean

gpg
[INFO]           sign-release-best-practices/pom.xml .............. SUCCESS (2.653 s)

bc
[INFO]           sign-release-best-practices/pom.xml .............. SUCCESS (2.237 s)

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.

3 participants