Add an option to disable keyring file #3145
Conversation
vendor/vendor.json
Outdated
| @@ -675,11 +675,11 @@ | |||
| "revisionTime": "2017-05-25T23:15:04Z" | |||
| }, | |||
| { | |||
| "checksumSHA1": "ZkJRgexeNzNZzpw6YnedwoJl7pE=", | |||
| "checksumSHA1": "sdjkMflUxYtL/l7n7wYPS0Xj5c0=", | |||
There was a problem hiding this comment.
Curious, why did vendor.json and the above two files have to change for this, they seem unrelated to the change being made?
| @@ -887,8 +887,10 @@ func (a *Agent) makeServer() (*consul.Server, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| if err := a.setupKeyrings(config); err != nil { | |||
| return nil, fmt.Errorf("Failed to configure keyring: %v", err) | |||
| if !a.config.DisableKeyringFile { | |||
There was a problem hiding this comment.
Minor readability thing - Any reason why this config param is not enableKeyRingFile, set to false by default? I find negative boolean feature flags always take a couple of seconds more to follow when you are reading a config file that has them, because its like if its set to true the file won't be written
There was a problem hiding this comment.
This isn't a bad idea since we are creating the config and not worrying about BC. If we switch to a bool* we could make this EnableKeyringFile and default it to true.
There was a problem hiding this comment.
I'm ok leaving it disable too, since it defaults to enabled and we have a lot of other things like that in the config. I'll leave it up to you @kyhavlov - either way works.
kyhavlov
force-pushed
the
disable-keyring-file
branch
from
c1c6202
to
1a3040b
Compare
kyhavlov
force-pushed
the
disable-keyring-file
branch
from
1a3040b
to
df449ab
Compare
| @@ -887,8 +887,10 @@ func (a *Agent) makeServer() (*consul.Server, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| if err := a.setupKeyrings(config); err != nil { | |||
| return nil, fmt.Errorf("Failed to configure keyring: %v", err) | |||
| if !a.config.DisableKeyringFile { | |||
There was a problem hiding this comment.
This isn't a bad idea since we are creating the config and not worrying about BC. If we switch to a bool* we could make this EnableKeyringFile and default it to true.
| @@ -318,6 +318,10 @@ func TestDecodeConfig(t *testing.T) { | |||
| c: &Config{EnableSyslog: true}, | |||
| }, | |||
| { | |||
| in: `{"disable_keyring_file":true}`, | |||
There was a problem hiding this comment.
Need to update the website with the new option.
Also disables keyring file in dev mode.
Closes #835, #2630, and #2784.