Issue/#114 cli editor support by timothysparg · Pull Request #121 · atomist/cli

timothysparg · 2019-10-24T19:28:56Z

Resolves #114

timothysparg · 2019-10-24T19:30:44Z

Is there a better way to structure this for testing?

…itor-support

ddgenome

It seems the edit capability is neither part of kube-encrypt or kube-decrypt, since what you really want is a round trip, decrypt-edit-encrypt. Maybe it should be its own command, kube-edit.

Co-Authored-By: David Dooling <dooling@gmail.com>

ddgenome · 2019-10-24T20:42:58Z

Regarding test, for starters you could set the editor to true and ensure that the round trip leaves the content unmodified. You could also test using sed, if you are on a system likely to have it.

timothysparg · 2019-10-25T07:08:20Z

It seems the edit capability is neither part of kube-encrypt or kube-decrypt, since what you really want is a round trip, decrypt-edit-encrypt. Maybe it should be its own command, kube-edit.

ok, let me rework as it's own command and see how that looks.

timothysparg · 2019-10-29T18:56:23Z

@ddgenome do you mind having a look and seeing if this is heading in the right direction?

ddgenome

It looks like a good start. I couple guiding questions that may make the implementation easier.

timothysparg · 2019-10-31T12:40:09Z

It looks like a good start. I couple guiding questions that may make the implementation easier.

With regard to the questions about the cli options, would it then make sense to introduce a --save option, or would we leave that up to the user ie kube-edit --file=blah --secret-key=🔑 >> blah

ddgenome · 2019-10-31T13:18:23Z

When would you not want to save? I am thinking the kube-edit command takes an encoded and optionally encrypted secret spec, optionally decrypts, decodes, writes that to a temp file, opens an editor on that file, waits for the editor to exit, reads the file back in, encodes, optionally encrypts, and writes that to the original file.

This pattern is useful for GitOps users, so you can always recover the original contents of the secret spec from the Git history.

timothysparg · 2019-10-31T18:19:31Z

ah ok, I missed that initially - thought we would just be writing to std out.

timothysparg · 2019-11-09T11:14:49Z

@ddgenome mind having a look when you have a chance?

ddgenome

This is looking good. Great tests. A few minor things. Thanks for sticking with it!

ddgenome · 2019-11-10T23:24:15Z

+            parameterName: "file",
+            describe: "Edit Kubernetes secret data values from secret spec file",
+            type: "string",
+            required: true,


Sorry I was not clearer on this before. I think having a required option is an oxymoron. If people have to provide it, why make them type --file=? Just grab it from argv._ and complain if it is not there.

I've changed this to use a positional argument, which seems to work pretty well

ddgenome · 2019-11-10T23:25:20Z

+    try {
+        secret = await handleSecretParameter(opts);
+    } catch (e) {
+        print.error(`Failed to load secret spec from file '${opts.file}': ${e.message}`);


This error message is not accurate anymore. Perhaps all the try/catch should be in the handleSecretParameter functions.

That try/catch is used more for exit handling than anything else, would then need to move exit (code) handling into the handleSecretParameter function, which feels a little inconsistent as the crypt method also uses the same pattern for exit code handling

Since only the reading of the file can throw an error, it seems to make sense to handle that issue there and use a different mechanism, e.g., returning undefined, to signal a failure of the function that then results in exit status handling.

ah ok, that makes sense. Will update accordingly

Co-Authored-By: David Dooling <dooling@gmail.com>

…rg/cli into issue/#114-cli-editor-support

timothysparg · 2019-11-13T08:07:54Z

@ddgenome do you mind reviewing again?

ddgenome · 2019-11-22T13:39:27Z

+    try {
+        secret = await handleSecretParameter(opts);
+    } catch (e) {
+        print.error(`Failed to load secret spec from ${opts.file ? `'${opts.file}'` : "--file or --literal"}: ${e.message}`);


If opts.file is not truthy, why would you say "--file or --literal"?

ddgenome · 2019-11-22T13:41:48Z

+    try {
+        secret = await handleSecretParameter(opts);
+    } catch (e) {
+        print.error(`Failed to load secret spec from file '${opts.file}': ${e.message}`);


Since only the reading of the file can throw an error, it seems to make sense to handle that issue there and use a different mechanism, e.g., returning undefined, to signal a failure of the function that then results in exit status handling.

ddgenome

I'm still not sold on the file error handling, but if you feel strongly that it should be this way, I'm okay with merging.

…ests

timothysparg · 2019-11-25T13:29:06Z

@ddgenome

I've updated the PR with the latest changes:

handleSecretParameter now returns undefined and internally handles the try/catch
Added conflicts: "literal" and conflicts: "file" cli restrictions for kube-encrypt/decrypt
Added test/kubeDecrypt.cli.test.ts and test/kubeEncrypt.cli.test.ts

I would really like to add some interactive cli testing ( the suppose package looks particularly promising for this ) - but I think that is for another pull request

ddgenome

LGTM

[atomist:generated]

timothysparg added 2 commits October 24, 2019 20:45

Add --edit functionality to kube encrypt/decrypt

a3192d3

Resolves #114

comments

fb9571c

Merge remote-tracking branch 'upstream/master' into issue/#114-cli-ed…

c122de3

…itor-support

ddgenome reviewed Oct 24, 2019

View reviewed changes

Comment thread lib/kubeCrypt.ts Outdated

Comment thread lib/kubeCrypt.ts Outdated

Comment thread lib/kubeCrypt.ts Outdated

Comment thread package.json Outdated

Comment thread lib/kubeCrypt.ts Outdated

Update lib/kubeCrypt.ts

216ca5b

Co-Authored-By: David Dooling <dooling@gmail.com>

ddgenome self-assigned this Oct 24, 2019

timothysparg added 8 commits October 27, 2019 21:21

Extracted functions into common file

6de1df1

extract printSecret

ab23128

Introduced KubeEdit

eea86cb

remove tempy

151b0b2

Update kube-edit parameter descriptions

bb1f0f2

update kube-edit parameter descriptions

87ef1f6

remove openEditor from KubeCryptOptions

b34b034

keep kube{Crypt,Utils} history

522c721

timothysparg commented Oct 29, 2019

View reviewed changes

Comment thread lib/kubeEdit.ts Outdated

ddgenome reviewed Oct 29, 2019

View reviewed changes

Comment thread index.ts Outdated

Comment thread index.ts Outdated

Comment thread index.ts Outdated

Comment thread lib/kubeEdit.ts Outdated

Comment thread lib/kubeEdit.ts Outdated

timothysparg added 7 commits November 4, 2019 20:27

Use external-editor to open default editor

babe09b

Remove unused cli param and write back to file

3ab3ed4

Refactor cryptEncode to base64 and crypt

40b5043

Resolve linting error

5dfa262

Introduce kubeEdit tests

ee55e35

refactor functions back to kubeCrpyt

988371d

update invalid file provided test

b09db8d

ddgenome reviewed Nov 10, 2019

View reviewed changes

timothysparg and others added 4 commits November 11, 2019 08:30

Update lib/kubeEdit.ts

c6e37ca

Co-Authored-By: David Dooling <dooling@gmail.com>

Use positional arguments for kube-edit

2cc3fbc

Merge branch 'issue/#114-cli-editor-support' of github.com:timothyspa…

c795efd

…rg/cli into issue/#114-cli-editor-support

Adjust handleSecretParameter error message

b7f5f7a

import preferred to require

ec150fe

ddgenome reviewed Nov 22, 2019

View reviewed changes

timothysparg added 6 commits November 22, 2019 20:21

Refactored error handling into handleSecretParameter and introduced t…

ee42133

…ests

File and literal are mutually exclusive

d7d8b86

Refactored handleSecretParameter and printSecret

937ddf3

reorder wrapLiteral function parameters

88338e7

Refactor wrapliteral function

e2d4992

Add kube{Encrypt,Decrypt} cli tests

445bc9a

ddgenome added auto-merge-method:squash Auto-merge with squash and merge auto-merge:on-approve Auto-merge on review approvals changelog:added Add this issue or pull request to added changelog section labels Nov 25, 2019

ddgenome approved these changes Nov 25, 2019

View reviewed changes

ddgenome merged commit 2831483 into atomist:master Nov 25, 2019

atomist Bot pushed a commit that referenced this pull request Nov 25, 2019

Changelog: #121 to added

8b704fa

[atomist:generated]

Conversation

timothysparg commented Oct 24, 2019

Uh oh!

timothysparg commented Oct 24, 2019

Uh oh!

ddgenome left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ddgenome commented Oct 24, 2019

Uh oh!

timothysparg commented Oct 25, 2019

Uh oh!

timothysparg commented Oct 29, 2019

Uh oh!

Uh oh!

ddgenome left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timothysparg commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddgenome commented Oct 31, 2019

Uh oh!

timothysparg commented Oct 31, 2019

Uh oh!

timothysparg commented Nov 9, 2019

Uh oh!

ddgenome left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timothysparg commented Nov 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddgenome left a comment

Choose a reason for hiding this comment

Uh oh!

timothysparg commented Nov 25, 2019

Uh oh!

ddgenome left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

timothysparg commented Oct 31, 2019 •

edited

Loading