-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add kv subcommand and CLI #2360
Conversation
The failing test seems unrelated and is also failing on master:
|
// base64 encoded upon transport. | ||
Value []byte | ||
|
||
// Session is a string representing the name of the session. Any other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/name/ID/g
CreateIndex uint64 | ||
|
||
// ModifyIndex is used for the Check-And-Set operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd change this to:
ModifyIndex is used for the Check-And-Set operations and can also be fed back into the WaitIndex of the QueryOptions in order to perform blocking queries.
|
||
-modify-index=<int> Unsigned integer representing the ModifyIndex of the | ||
key. This is often combined with the -cas flag, but it | ||
can be specified for any key. The default value is 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consul will ignore this on any other key operations (you can't manually set ModifyIndex) so I'd just change this to:
This is used in combination with the -cas flag.
You might even be able to just do -cas= but that might not be very intuitive.
ModifyIndex: *modifyIndex, | ||
} | ||
|
||
// If the user did not supply a -modify-index, but wants a check-and-set, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just fail out - for a CAS they really need to supply an index.
output := ui.OutputWriter.String() | ||
for key, value := range keys { | ||
if !strings.Contains(output, key+":"+value) { | ||
t.Fatalf("bad %#v missing %q", output, key+"adafa") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"adafa"?
Usage: consul kv put [options] KEY [DATA] | ||
|
||
Writes the data to the given path in the key-value store. The data can be of | ||
any type, but it will be transported as a base64-encoded string for safe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically the PUT doesn't Base64 encode because there's no JSON body.
lock. The session must already exist and be specified | ||
via the -session flag. The default value is false. | ||
|
||
-cas Perform a Check-And-Set operation. If this value is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as delete, if they CAS they must supply an index, otherwise it should fail.
|
||
-modify-index=<int> Unsigned integer representing the ModifyIndex of the | ||
key. This is often combined with the -cas flag, but it | ||
can be specified for any key. The default value is 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only makes sense for a CAS, otherwise Consul will ignore it.
|
||
switch { | ||
case *cas: | ||
// If the user did not supply a -modify-index, but wants a check-and-set, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should remove this.
|
||
$ consul kv put config/redis/maxconns 5 | ||
|
||
The data can also be consumed from a file on disk by prefixing with the "@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also document -
for stdin.
key = key[1:] | ||
} | ||
|
||
// If the key is empty and we are not doing a recursive delete, this is an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You think an empty recursive delete might be worth a special flag that protects people from fat fingers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. I'm leaning towards "no" for this initial pass, since ideally you are using ACLs which prevent this from happening and/or backing up your Consul data.
#### KV Delete Options | ||
|
||
* `-cas` - Perform a Check-And-Set operation. If this value is specified without | ||
-modify-index, the key will first be fetched and the resulting ModifyIndex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will need an update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome - noted some small things and then I think it's good to merge.
will be used on the next query. The default value is false. | ||
|
||
* `-modify-index=<int>` - Unsigned integer representing the ModifyIndex of the | ||
key. This is often combined with the -cas flag, but it can be specified for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"any key" part needs an update.
``` | ||
|
||
It is also possible to have Consul fetch the current ModifyIndex before making | ||
the query, by omitting the `-modify-index` flag. If the data is changed between |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should delete this (since the user can't see the data it won't be safe).
|
||
Command: `consul kv get` | ||
|
||
The `kv get` command is used to retrieves the value from Consul's key-value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"is used to retrieves"
Token: *token, | ||
} | ||
|
||
switch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we used /v1/txn under the hood for these operations we could print out the resulting CreateIndex and ModifyIndex on success, which would be kind of neat for trying to do CAS stuff manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I'm not super familiar with that stuff. Did you want me to do something here or were ya just thinking out loud?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add it later - mostly thinking out loud.
Command: `consul kv put` | ||
|
||
The `kv put` command writes the data to the given path in the key-value store. | ||
The data can be of any type, but it will be transported as a base64-encoded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically Base64 is only for the read side.
operation will create the key and obtain the lock. The session must already | ||
exist and be specified via the -session flag. The default value is false. | ||
|
||
* `-cas` - Perform a Check-And-Set operation. If this value is specified without |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz update
Success! Data written to: redis/config/connections | ||
``` | ||
|
||
It is also possible to have Consul fetch the current ModifyIndex before making |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz remove
@sethvargo that failing test is a known flaky one unrelated to this - sorry about that. |
@@ -36,9 +36,9 @@ type KVPair struct { | |||
// base64 encoded upon transport. | |||
Value []byte | |||
|
|||
// Session is a string representing the name of the session. Any other | |||
// Session is a string representing the ID of the session. Any othe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"othe"
Based on discussions with @slackpad, some of the values in the KVPair are ReadOnly. This commit updates the docs to reflect that.
31832f0
to
40de585
Compare
This implements the
kv
subcommand CLI for reading, writing, and deleting keys. This pass does not support the transaction-based keys yet, but that could be implemented in a later pass. The framework is in place for adding new commands and formats pretty easily./cc @slackpad @ryanuber @armon