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

Ability to pass GPG arguments to publish #851

Merged
merged 1 commit into from
May 6, 2020
Merged

Conversation

joan38
Copy link
Collaborator

@joan38 joan38 commented May 5, 2020

Not sure if we are ok to break the interface here. If not I can probably add back gpgPassphrase and gpgKeyName with a @deprecated and wire that to gpgArgs.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

I like this more than #753.

Now, that you've touched these method signatures, can you please also add scaladoc to the method and it's parameters?

val command = "gpg" ::
optionFlag("--passphrase", maybePassphrase) ++ optionFlag("-u", maybeKeyName) ++
Seq("--batch", "--yes", "-a", "-b", fileName)
val command = Seq("gpg", "--batch", "--yes", "-a", "-b", fileName) ++ args
Copy link
Member

Choose a reason for hiding this comment

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

Wondering, if we should move these defaults into the default param to gpgArgs and document it's meaning. It's not a change request, just a thought. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking the same but then people need to be aware that if there specify the --gpgArgs they need to provide these too to keep them.

Copy link
Member

Choose a reason for hiding this comment

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

We could add a hint in the scaladoc, too. But having these hard-coded args screams for future maintenance work, as seen in the past (#345, #530, #753). We could also rename the parameter to alternativeGpgArgs or overrideGpgArgs to indicate the fact that it replaced all defaults.

Copy link
Member

Choose a reason for hiding this comment

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

What about an accessor to the defaults to make reusing easy:

val defaultGpgArgs = ...
def publish(...,  gpgArgs: Seq[String] = defaultGpgArgs, ...)

And a simple example in the docs, showing how to add a passphrase using the defaultsArgs + the passphrase args

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good to me. What do you think @jodersky?

Copy link
Member

Choose a reason for hiding this comment

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

Looks good! I agree with @lefou's suggestion too. Thanks for working on this!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Change pushed

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me

@lefou lefou merged commit 4f35588 into com-lihaoyi:master May 6, 2020
@lefou lefou added this to the 0.7.0 milestone May 6, 2020
@lihaoyi
Copy link
Member

lihaoyi commented May 7, 2020

This has broken publishing on the master branch:

Unknown arguments: "--gpgPassphrase" "[secure]"

@joan38 can you take a look?

@joan38
Copy link
Collaborator Author

joan38 commented May 7, 2020

@lihaoyi You need to pass --gpgArgs '--passphrase [secure]' instead of --gpgPassphrase [secure]

This is breaking change in terms of interface that is why I mentioned:

Not sure if we are ok to break the interface here. If not I can probably add back gpgPassphrase and gpgKeyName with a @deprecated and wire that to gpgArgs.

Let me know if you got it solved.

@joan38 joan38 deleted the gpg branch May 9, 2020 05:26
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.

None yet

4 participants