Skip to content

feat: add GPG key management commands#3822

Merged
samcoe merged 6 commits intocli:trunkfrom
owenvoke:feature/gpg-key
Oct 18, 2021
Merged

feat: add GPG key management commands#3822
samcoe merged 6 commits intocli:trunkfrom
owenvoke:feature/gpg-key

Conversation

@owenvoke
Copy link
Contributor

@owenvoke owenvoke commented Jun 11, 2021 •
edited
Loading

This adds two new subcommands (gpg-key list and gpg-key add) based off of the ssh-key subcommands. I've never worked with Go before, so hope this is correct.

Related to #1755 (comment)

$ gh gpg-key add ~/gpg_key.asc
✓ GPG public key added to your account

$ gh gpg-key
Manage GPG keys registered with your GitHub account

USAGE
  gh gpg-key <command> [flags]

CORE COMMANDS
  add:        Add a GPG key to your GitHub account
  list:       Lists GPG keys in your GitHub account

Currently gh gpg-key list will show the output using the following columns:

  • GPG key ID
  • Expiry date
  • Public key ID (truncated in the middle if necessary)

I wasn't sure whether to included the date it was uploaded, or anything else as it seemed to get a bit cluttered.

I've temporarily created a GitHub CLI extension for this, however I'd much rather it be in core. 👍🏻

@mislav mislav added the enhancement a request to improve CLI label Aug 4, 2021
@samcoe samcoe self-requested a review September 13, 2021 19:58
samcoe
samcoe approved these changes Sep 13, 2021
View reviewed changes
Copy link
Contributor

@samcoe samcoe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry @owenvoke for taking so long to review this. Thanks for the submission, the code looked great! I pushed some small 💅 changes as well as merged in trunk and updated the code to use the new v2 import statements since v2 was recently released.

@owenvoke
Copy link
Contributor Author

owenvoke commented Sep 13, 2021

Thank you so much @samcoe, that's brilliant! And no worries about the review time, glad I could contribute to the CLI. 🥳

samcoe
samcoe reviewed Sep 13, 2021
View reviewed changes
mislav
mislav previously requested changes Sep 14, 2021
View reviewed changes
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on! I have minor output suggestions

pkg/cmd/gpg-key/list/list.go
for _, gpgKey := range gpgKeys {
t.AddField(gpgKey.Emails.String(), nil, nil)

t.AddField(gpgKey.KeyId, nil, nil)
Copy link
Contributor

@mislav mislav Sep 14, 2021

Choose a reason for hiding this comment

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

Should the ID field come first?

Copy link
Contributor Author

@owenvoke owenvoke Sep 14, 2021

Choose a reason for hiding this comment

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

I'd say it probably should do.

Copy link
Contributor

@samcoe samcoe Sep 14, 2021

Choose a reason for hiding this comment

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

I disagree here, I think the email associated with the key is how most people will recognize which key is which if there are multiple associated with an account since they don't have names like SSH keys. Additionally the UI on the GitHub settings page, as well as other GPG keystores I have seen, usually list email first.

@samcoe samcoe requested a review from mislav September 14, 2021 17:56
@samcoe samcoe dismissed mislav’s stale review October 18, 2021 18:36

Addressed PR feedback

@samcoe samcoe merged commit a02d423 into cli:trunk Oct 18, 2021
@owenvoke owenvoke deleted the feature/gpg-key branch October 18, 2021 19:11
@owenvoke
Copy link
Contributor Author

owenvoke commented Oct 18, 2021

Thank you @samcoe!

@owenvoke owenvoke mentioned this pull request Oct 18, 2021
Closed
@jenkins-infra-bot jenkins-infra-bot mentioned this pull request Nov 19, 2021
Merged
@Janiel13

This comment was marked as spam.

Sign in to view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mislav mislav Awaiting requested review from mislav

1 more reviewer

@samcoe samcoe samcoe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@samcoe samcoe

Labels

enhancement a request to improve CLI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@owenvoke @Janiel13 @mislav @samcoe @Jamacna