fix(cluster connect): print access commands separately for services #1327
Conversation
|
Still verifying but code changes look good! |
One query, things will get tricky when context mode is not used, while we can modify the command for registry, the issue will persist with Kafka. |
Not sure I understand. Is there still some work to be done? |
pkg/cluster/connect.go
Outdated
| @@ -218,6 +218,8 @@ func (c *KubernetesClusterAPIImpl) createServiceAccountSecretIfNeeded(namespace | |||
| localize.NewEntry("ClientID", serviceAcct.GetClientId()), | |||
| )) | |||
|
|
|||
| currentService.PrintAccessCommands(serviceAcct.GetClientId()) | |||
There was a problem hiding this comment.
My own feeling is that this does not belong in this package and should be printed from the connect command. This package is a Kubernetes API client and by print stuff to the console it is mixing responsibility layers.
There was a problem hiding this comment.
I haven't seen requirement to have this as kube client. I know we have this interface for it but IMHO this is slightly over-designing something that is very tightly related to CLI command. There are many places in services where we do the same, use context etc.
Sole purpose of this package is to serve as execution layer for commands.
There was a problem hiding this comment.
The thing that highlighted this for me is that we are passing down currentService as a new function argument for the sole purpose of printing the access commands. It does not relate to the core purpose of the function (creating a service when needed) and goes against the single-responsibility principle.
There was a problem hiding this comment.
Funny. Rama wanted it this way and I have recomended to do it in function also quoting single responsibility rule (dealing with service accounts)
#opinions :)
There was a problem hiding this comment.
@rkpattnaik780 to make final call on this
There was a problem hiding this comment.
Yeah, same result at the end of the day :)
There was a problem hiding this comment.
I agree with the method part, have updated the method to return a client ID if service account is created and print from connect command.
| @@ -89,11 +89,15 @@ func (api *KubernetesClusterAPIImpl) ExecuteConnect(connectOpts *v1alpha.Connect | |||
| return kubeclient.TranslatedKubernetesErrors(api.CommandEnvironment, err) | |||
| } | |||
|
|
|||
| err = api.createServiceAccountSecretIfNeeded(currentNamespace, currentService) | |||
| clientID, err := api.createServiceAccountSecretIfNeeded(currentNamespace) | |||
Display commands required to grant access to the service-account.
Verification Steps
It should print command to give access to the the service registry
Type of change
Checklist