Database updates #4787
Database updates #4787
Conversation
| if pluginNameRaw, ok := data.GetOk("plugin_name"); ok { | ||
| config.PluginName = pluginNameRaw.(string) | ||
| } else if req.Operation == logical.CreateOperation { | ||
| config.PluginName = data.Get("plugin_name").(string) |
There was a problem hiding this comment.
In all of these else if blocks (in path_roles.go too), I think you can just initialize to the appropriate zero value ("" in this case). We already know the parameter doesn't exist and don't need to check it again.
There was a problem hiding this comment.
The reason we usually use this is in case we decide to provide/change a default value later. Instead of setitng the default in the schema and here, we just set it in the schema.
There was a problem hiding this comment.
Ah, TIL... That makes sense.
| // Test existence check and an update to a single connection detail parameter | ||
| { | ||
| configData := map[string]interface{}{ | ||
| "connection_url": "sample_convection_url", |
There was a problem hiding this comment.
convection->connection (various places)
There was a problem hiding this comment.
Nope, it's on purpose! Note that the previous check has connection and this has convection. The point is to check that it does indeed update, when other values that haven't been set stay the same.
There was a problem hiding this comment.
Perhaps this is too subtle, I just enjoyed changing it to another real word with a single char substitution. :-)
| @@ -554,8 +727,8 @@ func TestBackend_connectionCrud(t *testing.T) { | |||
| } | |||
|
|
|||
| delete(resp.Data["connection_details"].(map[string]interface{}), "name") | |||
| if !reflect.DeepEqual(expected, resp.Data) { | |||
| t.Fatalf("bad: expected:%#v\nactual:%#v\n", expected, resp.Data) | |||
| if diff := deep.Equal(resp.Data, expected); diff != nil { | |||
There was a problem hiding this comment.
What's the criteria for switching to deep.Equal()? It's fine, though I also see new code using reflect.Equal() in this PR.
There was a problem hiding this comment.
The new code is just where I've copied and pasted a test; where I've added further conditions I've tended towards deep.Equal.
There's no real criteria, I just tend to use it for new statements.
|
Minor comments, otherwise LGTM! |
| dbName = role.DBName | ||
| statements = role.Statements | ||
| } else { | ||
| if dbNameRaw, ok := req.Secret.InternalData["db_name"]; !ok { |
There was a problem hiding this comment.
I wonder if we should just keep the current behavior of returning "role not found" if internal data does not have the fields.
There was a problem hiding this comment.
You mean, instead of the more specific error messages? Having those makes debugging easier because we know exactly where things are failing.
|
This is awesome. Thanks! |
|
👍 |
shot at working if the role has been deleted
Fixes #3544
Fixes #4782