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

Feature - allow signing to be specified in settings.xml #50

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

adamretter
Copy link

@adamretter adamretter commented Jun 5, 2020

This PR takes a feature from jdeb which allows the signing information to be specified in an active profile within ~/.m2/settings.xml.

This makes configuring the plugin for externalising the sensitive signature config options simpler, as custom property names no longer have to be explicitly specified in the pom.xml. Of course you can still specify custom property names if you wish ;-)

Simply adding the following to your your ~/.m2/settings.xml should be enough:

<profiles>

       <profile>
          <id>rpm-signing</id>
          <properties>
            <rpm.keyringFile>/home/me/.gnupg/secring.gpg</rpm.keyringFile>
            <rpm.keyId>AA540227</rpm.keyId>
            <rpm.passphrase>top-secret</rpm.passphrase>
          </properties>
        </profile>

</profiles>


<activeProfiles>

       <activeProfile>rpm-signing</activeProfile>

</activeProfiles>

The rpm. prefix of the property names can be customised through the plugin configuration element signCfgPrefix

@adamretter adamretter force-pushed the feature/default-signature-properties branch from 7172492 to 792e01c Compare June 7, 2020 11:39
@adamretter adamretter changed the title Add property names for signature config Feature - allow signing to be specified in settings.xml Jun 7, 2020
@ctron
Copy link
Owner

ctron commented Jun 9, 2020

Thanks for the PR. This makes total sense. There are a few "style" comments I will add, I hope you don't mind 😀

I am just wondering, should it already be possible to configure the via properties using rpm.signature?

import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.*;
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer to have expanded imports.

Copy link
Author

@adamretter adamretter Jun 9, 2020

Choose a reason for hiding this comment

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

Okay sure. Apologies, my IDE (IntelliJ) insists on doing these automated "improvements" for me.

* The signing variables are: keyId, keyringFile, passphrase, hashAlgorithm.
*/
@Parameter(defaultValue = "rpm.")
private String signCfgPrefix;
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer a non-abbreviated name, e.g. signConfigurationPrefix. And wouldn't it make sense to allow the user to set this via properties as well?

Copy link
Author

Choose a reason for hiding this comment

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

I can certainly do that. I just took the existing syntax from jdeb, but I can update it...

@@ -785,6 +796,47 @@ public void execute () throws MojoExecutionException, MojoFailureException
provider.apply ( builder );
}

// load any default signing properties from settings.xml
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe wrap this section up in another method?

return Collections.emptyMap();
}

final Map<String, String> map = new HashMap<String, String>();
Copy link
Owner

Choose a reason for hiding this comment

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

IIRC we are on Java 8. So you can drop the type arguments on the constructor:

Suggested change
final Map<String, String> map = new HashMap<String, String>();
final Map<String, String> map = new HashMap<>();

}

final Map<String, String> map = new HashMap<String, String>();
final Set<String> activeProfiles = new HashSet<String>(activeProfilesList);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
final Set<String> activeProfiles = new HashSet<String>(activeProfilesList);
final Set<String> activeProfiles = new HashSet<>(activeProfilesList);

@ctron
Copy link
Owner

ctron commented Jun 9, 2020

Re-iterating over the PR, wouldn't it be possible to find a solution which doesn't require to manually iterate over the settings and profiles?

@adamretter
Copy link
Author

adamretter commented Jun 9, 2020

@ctron I did actually try such an approach first but I could not get it to work, which is why we ended up with iterating over the properties.

@adamretter
Copy link
Author

@ctron I pushed a commit which I think addresses your comments.

@dwalluck
Copy link
Collaborator

The rpm. prefix of the property names can be customised through the plugin configuration element signCfgPrefix

I kind of wonder why it can't use the existing gpg properties (described here) for this (although I understand why you might want different ones).

I guess putting it in settings has transparent support for encrypted passwords?

I don't really see the point in making the prefix configurable as all the other properties using an rpm. prefix and all the properties should use the same prefix by convention.

@ctron
Copy link
Owner

ctron commented Jun 15, 2020

The rpm. prefix of the property names can be customised through the plugin configuration element signCfgPrefix

I don't really see the point in making the prefix configurable as all the other properties using an rpm. prefix and all the properties should use the same prefix by convention.

I tend to agree here. Making it configurable, may sound nice for some niche use cases, but adds some complexity that I am not sure, is worth it.

I completely understand the use case, but would prefer something that works without manually iterating over the settings.

@ctron
Copy link
Owner

ctron commented Jun 15, 2020

I did a quick check: Using the properties from MavenProject#getProperties you already have everything expanded. I also checked the maven resources filtering system, they basically take the project properties, overwrite this with the system properties, and then the user properties. I think this should be much simpler, and re-use all that maven does anyway:

@Mojo(name = "sayhi")
public class MyMojo extends AbstractMojo {

    @Parameter(property = "project", readonly = true, required = true)
    private MavenProject project;

    @Parameter(property = "session", readonly = true, required = true)
    private MavenSession session;

    @Override
    public void execute() throws MojoExecutionException {
        Properties p = new Properties();
        p.putAll(project.getProperties());
        p.putAll(session.getSystemProperties());
        p.putAll(session.getUserProperties());
        for ( Map.Entry<?, ?> entry : p.entrySet()) {
            getLog().info(String.format("%-40s: %s", entry.getKey(), entry.getValue()));
        }
    }
}

@lburja
Copy link
Contributor

lburja commented Jan 7, 2021

Any chance for this to be merged? It looks like a nice improvement over configuring signing inside the pom.xml

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