Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

feat: add 'duffle creds edit' #356

Merged
merged 2 commits into from
Nov 8, 2018

Conversation

technosophos
Copy link
Member

Add an editor for editing the credentialsets that exist in your home directory.

This delegates all of the heavy lifting to the Survey module, which provides a cross-platform editor feature.

Closes #245

@technosophos technosophos self-assigned this Nov 5, 2018
@technosophos technosophos force-pushed the feat/245-credentials-edit branch 2 times, most recently from ee4ce05 to d509068 Compare November 5, 2018 22:02
Add an editor for editing the credentialsets that exist in your home directory.

This delegates all of the heavy lifting to the Survey module, which provides a cross-platform editor feature.

Closes #245
@technosophos technosophos force-pushed the feat/245-credentials-edit branch from d509068 to 29fc504 Compare November 5, 2018 22:04
Long: credentialEditDesc,
RunE: func(cmd *cobra.Command, args []string) error {
edit.home = home.Home(homePath())
edit.name = args[0]
Copy link
Member

Choose a reason for hiding this comment

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

This should error out if the provided credential name is empty - right now, it simply exits, without any additional information.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♂️ Ugh... I forgot to set the arg count flag. Thx. I'll fix that.

Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

As per the comment, duffle creds edit should error out when no credential set name is provided.

@radu-matei
Copy link
Member

radu-matei commented Nov 6, 2018

A couple of additional comments regarding the editor used:

  • on Windows, the YAML file is not correctly formatted, making rather difficult to read the file (so much for the human readable part) - but I suspect this is the fault of Notepad, as printing the file to the console shows it correctly formatted

  • the editor can be changed using the EDITOR or VISUAL environment variables - however, choosing VS Code does not work when the editor is already opened (the survey library doesn't correctly wait for VS Code and deletes the temporary file from disk) - this is consistent across Windows and Linux (didn't have a chance to test on macOS)

  • VS Code Insiders does not work at all (it opens an empty file)

However, these issues would have to be fixed upstream, in the survey library, and should not prevent this PR from getting merged.

@technosophos
Copy link
Member Author

I can confirm that when I did EDITOR="code -n" it did not work on Mac.

@technosophos
Copy link
Member Author

Ready for re-review

@@ -36,6 +36,7 @@ func newCredentialEditCmd(w io.Writer) *cobra.Command {
Use: "edit [NAME]",
Short: "edit an existing credential set",
Long: credentialEditDesc,
Args: cobra.ExactArgs(1),
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

LGTM

@technosophos technosophos merged commit dccbe2c into cnabio:master Nov 8, 2018
@technosophos technosophos deleted the feat/245-credentials-edit branch November 8, 2018 16:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants