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

feat: add seed phrase to backup-keys #128

Closed
wants to merge 1 commit into from
Closed

feat: add seed phrase to backup-keys #128

wants to merge 1 commit into from

Conversation

bashbunni
Copy link
Member

@bashbunni bashbunni commented Apr 27, 2022

Changes:

  • add melt seed phrase output for backups
  • make melt seed phrase look 🐰 cute 🌈 (UI changes)
    ui changes

Related Issues:

#116

@bashbunni bashbunni marked this pull request as ready for review April 27, 2022 21:49
@bashbunni
Copy link
Member Author

I copied a lot of code over from melt 😂 let me know if there's a nicer way I could organize things or if it's okay

@caarlos0
Copy link
Member

I copied a lot of code over from melt 😂 let me know if there's a nicer way I could organize things or if it's okay

I noticed hehe

I was thinking we can refactor there a bit to make it easier to use these parts as a lib too... will take a look there and ping you once its done :)

Comment on lines +214 to +222
pwderr := &ssh.PassphraseMissingError{}
if errors.As(err, &pwderr) {
pass, err := askKeyPassphrase(path)
if err != nil {
return "", err
}
return backup(path, pass)
}
return "", fmt.Errorf("could not parse key: %w", err)

Choose a reason for hiding this comment

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

Suggested change
pwderr := &ssh.PassphraseMissingError{}
if errors.As(err, &pwderr) {
pass, err := askKeyPassphrase(path)
if err != nil {
return "", err
}
return backup(path, pass)
}
return "", fmt.Errorf("could not parse key: %w", err)
if !errors.Is(err, &ssh.PassphraseMissingError{}) {
return "", fmt.Errorf("could not parse key: %w", err)
}
pass, err := askKeyPassphrase(path)
if err != nil {
return "", err
}
return backup(path, pass)

We can use errors.Is as we are not using pwderr for anything else.

By reversing the conditional here, we can reduce nesting, optionally we can encapsulate the logic in a new method askPassphraseAndBackup(path)

fmt.Fprint(os.Stderr, msg)
t, err := tty.Open()
if err != nil {
return nil, fmt.Errorf("could not open tty")

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("could not open tty")
return nil, errors.New("could not open tty")

This will save some allocations.

Copy link
Member

Choose a reason for hiding this comment

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

its actually missing the parent err:

Suggested change
return nil, fmt.Errorf("could not open tty")
return nil, fmt.Errorf("could not open tty: %w", err)

@caarlos0
Copy link
Member

alternative idea to this: an option to print the key to stdout, so users can pipe into melt (or something else) 🤔

caarlos0 added a commit to charmbracelet/melt that referenced this pull request Apr 28, 2022
review from charmbracelet/charm#128

Signed-off-by: Carlos A Becker <caarlos0@gmail.com>
@bashbunni bashbunni self-assigned this May 5, 2022
@bashbunni bashbunni added the enhancement New feature or request label May 5, 2022
caarlos0 added a commit to charmbracelet/melt that referenced this pull request May 6, 2022
review from charmbracelet/charm#128

Signed-off-by: Carlos A Becker <caarlos0@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants