Remove the newly generated key if sc add-gcp-broker command fails #133
Conversation
| return fmt.Errorf("error generating configs for GCP :: %v", err) | ||
| } | ||
|
|
||
| err = deployGCPBrokerConfigs(dir) | ||
| if err != nil { | ||
| // Clean up the newly generated key if the command failed. | ||
| cleanupNewKey(brokerSAEmail, key) | ||
| return fmt.Errorf("error deploying GCP broker configs :%v", err) | ||
| } |
There was a problem hiding this comment.
Group as a helper function, and then do something like:
if err := foo(a, b, c, ..); err != nil {
cleanupNewKey(brokerSAEmail, key)
return ...
}
There was a problem hiding this comment.
Do you mean to group getOrCreateVirtualBroker(), generateGCPBrokerConfigs(), and deployGCPBrokerConfigs() together?
In that case, I'd prefer to return the key to the NewXXXCommand method and let it handle the cleanup when the key is not empty.
There was a problem hiding this comment.
Calling cleanupNewKey in multiple places makes further change prone to introduce a bug. Some simple option of avoiding it is creating one helper function where you put all the cases that should call cleanupNewKey(). If you have other way to achieve it, please feel free to try that out.
There was a problem hiding this comment.
Will keep as is and clean up/refactor the entire tool when we have time. Tracked in #149.
| @@ -280,6 +287,25 @@ func httpAdapterFromAuthKey(keyFile string) (*adapter.HttpAdapter, error) { | |||
| return adapter.NewHttpAdapter(client), nil | |||
| } | |||
|
|
|||
| // cleanupNewKey removes the newly generated service account key. | |||
| func cleanupNewKey(email, key string) { | |||
There was a problem hiding this comment.
s/the newly generated/a given/
There was a problem hiding this comment.
This one is more than cleanup the "given key". It is to cleanup the given keyfile content in base64 encoding. I'll update the comment to be more specific.
| return | ||
| } | ||
|
|
||
| keyID := keyJson["private_key_id"].(string) |
There was a problem hiding this comment.
a chance of panic due to ".(string)" ?
There was a problem hiding this comment.
Tracked in #149.
| out, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| fmt.Printf("failed to delete service account key: %s : %v\n", string(out), err) | ||
| fmt.Printf("WARNING: Please clean up the key from service account %s", email) |
There was a problem hiding this comment.
"Error: failed to delete ..."
"Warning: please ..." ?
There was a problem hiding this comment.
The current error message is actually "freeform" and based on the experience of each developer. We need to do a bit design and clean it up. Tracked by #150.
Fixes #38