vmware-tanzu / tanzu-framework

Tanzu Framework provides a set of building blocks to build atop of the Tanzu platform and leverages Carvel packaging and plugins to provide users with a much stronger, more integrated experience than the loose coupling and stand-alone commands of the previous generation of tools.
Apache License 2.0
196 stars 193 forks source link

Guidance on use of unique flag within a group of commands #504

Open gjf20 opened 3 years ago

gjf20 commented 3 years ago

Describe the feature request The --scope flag and its inclusion on only one command in the Tanzu CLI Integrations Plugin.

Context There are 2 types of integration APIs that are conceptually similar and share the same commands.
1) Cluster integrations: creating a cluster integration installs that integration on a particular cluster. 2) Clustergroup integrations: creating a clustergroup integration creates a cluster integration on each of the clusters in the clustergroup

Currently, we invoke both of these APIs with the same commands, determining which API to invoke based on flags.
Ex: tanzu integration get INTEGRATION_NAME --cluster-name CLUSTER-NAME ... invokes the cluster integration API since the --cluster-name flag is present rather than the --cluster-group-name flag

Using this method of determining API based on flags works for get and delete because the CLI and APIs require the use of either the --cluster-name or --cluster-group-name flags to function properly. However, neither API's list command requires additional information in order to function, giving rise to a problem of determining which list API to call.

We propose to solve this problem by adding a --scope flag to tanzu integration list that can take the values 'cluster' and 'clustergroup'.

Before proceeding with this approach of having the --scope flag present on only the list command, I wanted to confirm that this unique flag is acceptable for the UX goals of the Tanzu CLI.

Describe alternatives you've considered We could add the --scope flag to the get and delete commands to make the flag more consistent across commands: Get: tanzu integration get INTEGRATION_NAME --scope cluster --cluster-name CLUSTER_NAME -m MANAGEMENT_CLUSTER_NAME -p PROVISIONER_NAME and tanzu integration get INTEGRATION_NAME --scope clustergroup --cluster-group-name CLUSTER_GROUP_NAME

Delete: tanzu integration delete INTEGRATION_NAME --scope cluster --cluster-name CLUSTER_NAME -m MANAGEMENT_CLUSTER_NAME -p PROVISIONER_NAME and tanzu integration delete INTEGRATION_NAME --scope clustergroup --cluster-group-name CLUSTER_GROUP_NAME

The drawback of this approach is that it becomes redundant for the user: specifying 'cluster' in the scope flag and still being required to specify 'cluster-name'

Affected product area (please put an X in all that apply)

Additional context

danfein commented 3 years ago

I am looking at this flag in isolation, without an understanding of the whole, so take anything I say with a grain of salt. :)

Having a flag for just list isn't a problem, however the behavior is interesting, combining resources under a shared command.

It sounds like '--scope' is acting like a filter, as in "show me a list of integrations, but just the cluster ones (or clustergroup ones).

Given that tanzu integration deals with multiple integration types, I would imagine an unflagged tanzu integrations list to include them both (two api calls it sounds like) with an option to filter if users only care about one type. It makes it possible to use the list command without a flag and for folks that want to see both types, it saves them needing to run list twice.

There might also be some context aware logic that changes the behavior depending on what the CLI is logged into (workload cluster, management cluster, etc). I don't know a lot about the integrations apis, but if someone is logged into a workload cluster, would they still be able to see clustergroup integrations? If not, the list command could skip calling that api when logged into one.

The apps plug in uses flags as filters in some cases, like: tanzu apps workload list --namespace NAME

Which could translate to something like tanzu integrations list --type cluster - which would filter down to one API, AND return results for one cluster in particular. (the flag could be type, scope, etc. I didn't see a prexisting flag that jumped out as being a natural fit.)

giri-varma commented 3 years ago

Adding a bit more context:

The Integration API is part of Tanzu Mission Control (TMC). It allows users to activate, configure and install integrations to other Tanzu SaaS services. Currently we support Tanzu Service Mesh (TSM) and Wavefront or Tanzu Observability (TObs). Swagger doc: https://code.vmware.com/apis/1079/tanzu-mission-control#/IntegrationResourceService

From the swagger documentation one can see that there are three different levels or scopes for the API:

  1. /v1alpha1/integrations provides the available integration options or types (TSM, TObs, etc.)
  2. /v1alpha1/organization/integrations allows activation/deactivation of said integrations with some organization level configurations
  3. /v1alpha1/clusters/{clusterName}/integrations allows installation and configuration of an integration on a single cluster

In future there will be a /v1alpha1/clustergroups/{clusterGroupName}/integrations which would allow installation and configuration of an integration to a fleet of clusters. This is also a pattern that we are intending to reuse for other plugins like policy which can be scoped/targeted towards a cluster, cluster group or workspace.

Given this context we have the following questions:

  1. What is your recommendation for the this kind of plugins?
  2. What do you feel about the --scope/--target flag to specify the scope in list commands? Also, we want to restrict to one scope at a time to avoid making multiple API calls at least in the initial version. So, we might want to make the --scope flag as mandatory.
  3. What do you think about extending this to other commands like get & delete? These commands do not need it since the scope can be inferred from the combination of identifier flags which will be unique to a target. The question is more about consistency between list and get/delete commands.
ali5ter commented 3 years ago

I was trying to imagine the various combinations of actions for the different levels of 'scope' and got this...

Org scope...

tanzu integration list [--scope org] lists available integrations for the org

tanzu integration create INTEGRATION_NAME [--scope org] ... activates specific integration for the organization

tanzu integration get INTEGRATION_NAME [--scope org] get specific integration detail for the organization

tanzu integration delete INTEGRATION_NAME [--scope org] deactivates specific integration for the organization

Cluster group scope...

tanzu integration list [--scope cluster-group] --cluster-group-name CLUSTER_GROUP_NAME lists activated/deactivated integrations for the cluster group

tanzu integration create INTEGRATION_NAME [--scope cluster-group] --cluster-group-name CLUSTER_GROUP_NAME activates specific integration for the cluster group

tanzu integration get INTEGRATION_NAME [--scope cluster-group] --cluster-group-name CLUSTER_GROUP_NAME get specific integration detail for the cluster group

tanzu integration delete INTEGRATION_NAME [--scope cluster-group] --cluster-group-name CLUSTER_GROUP_NAME deactivates specific integration for the cluster group

Cluster scope...

tanzu integration list [--scope cluster] --cluster-name CLUSTER_NAME -m MANAGEMENT_CLUSTER_NAME -p PROVISIONER_NAME lists activated/deactivated integrations for the cluster

tanzu integration create INTEGRATION_NAME [--scope cluster] --cluster-name CLUSTER_NAME -m MANAGEMENT_CLUSTER_NAME -p PROVISIONER_NAME activates specific integration for the cluster

tanzu integration get INTEGRATION_NAME [--scope cluster] --cluster-name CLUSTER_NAME -m MANAGEMENT_CLUSTER_NAME -p PROVISIONER_NAME get specific integration detail for the cluster

tanzu integration delete INTEGRATION_NAME [--scope cluster] --cluster-name CLUSTER_NAME -m MANAGEMENT_CLUSTER_NAME -p PROVISIONER_NAME deactivates specific integration for the cluster

@giri-varma is that correct?

I was wondering what the first level of scope was in the Swagger documentation and how that was different from the org scope.

It seems to me, at least for cluster group and cluster, that the identifier flag would have to be provided for list, create, get and delete actions and so the scope is implied.

Just a little confused about what /v1alpha1/integrations provides compared with /v1alpha1/organization/integrations.

giri-varma commented 3 years ago

Yes, the enumeration looks correct to me!

Just a little confused about what /v1alpha1/integrations provides compared with /v1alpha1/organization/integrations.

/v1alpha1/integrations is a static list of integrations with some general read-only info, that are available to be activated. /v1alpha1/organization/integrations is an editable list and gets populated as a user activates a plugin for the organization. Might take inputs like the endpoint of the integration for the organization.

It seems to me, at least for cluster group and cluster, that the identifier flag would have to be provided for list, create, get and delete actions and so the scope is implied.

For the list command, the --scope flag is indeed required. But for the create/get/delete commands, it can be inferred implicitly by the combination of flags without the --scope flag. This combination of flags is always unique to the resource/scope. E.g., cluster - tanzu integration get INTEGRATION_NAME --cluster-name CLUSTER_NAME -m MANAGEMENT_CLUSTER_NAME -p PROVISIONER_NAME cluster-group - tanzu integration get INTEGRATION_NAME --cluster-group-name CLUSTER_GROUP_NAME So, the question is about clarity vs simplicity (one less flag), if it makes sense.

ali5ter commented 2 years ago

Thanks for your patience and time chatting about this. This is what Dan and I propose:

This is explained specifically for the tanzu integrations command but, if this proposal is accepted, we'd like to document this pattern in the style guide.

giri-varma commented 2 years ago

Appreciate the detailed recommendation @ali5ter !

As an extension, I assume that to list all cluster integrations under a provisioner a user would run tanzu integrations list -m mgmt-cluster-01 -p provisioned-01. I think this could work for the TMC integrations command. I am not sure if it works for the similar TMC account credentials command where two types are part of the same resource hierarchy.

For example,

tanzu account credential list # org level creds
tanzu account credential list -m mgmt-cluster-01 # managment cluster creds or provisioner creds under a management cluster?
tanzu account credential list -m mgmt-cluster-01 -p prov-01 # provisioner creds

tanzu account credential list --all-management-clusters # under all management clusters
tanzu account credential list --all-provisioners # under all provisioners in all management clusters

Here tanzu account credential list -m mgmt-cluster-01 is not clear between the following two cases,

  1. list all management cluster credentials under a management cluster
  2. list all provisioner credentials under a management cluster
ali5ter commented 2 years ago

@giri-varma Good point. I think your assumption would be correct for integrations.

I'm thinking the combination of the following options might address the two cases for listing creds:

giri-varma commented 2 years ago

Cool! Think that works. @pgandigesang , @gjf20 what do you think?

@ali5ter Want to run one more query by you. Since we need a --all-<parent> boolean flag for every type of parent/target/scope that is supported, would adding a single flag say --scope/parent/target string flag as in the original proposal improve the UX? The users have lesser flags to remember. What do you think?

ali5ter commented 2 years ago

Great question @giri-varma

@danfein and I talked about the pro's and cons of using a single flag with different values versus unique flags. While, as you say, the former implements a single flag, our thought was there was customer benefit to the unique flag, such that it:

  1. Helps the user from having to manually type in the correct value without the benefit of autocomplete
  2. --all-<parent> provides a crisp representation of the option without having to depend on selecting an option value like scope, type, level, parent or target that may not be ideal.
  3. Uses prior art of the use of this pattern, e.g. --all-namespace, that our representative audience may be accustomed to.

cc: @danfein keep me honest here :)

giri-varma commented 2 years ago

@ali5ter I do not have much knowledge of existing art elsewhere other the K8s example. If you think this approach is clearer to the user, then I am onboard with it :) Thanks for helping figure out this complex use case.

@pgandigesang , @gjf20 Hope that unblocks us! Please let us know if we are missing something.

pgandigesang commented 2 years ago

This should unblock us, will make changes as suggested. Thanks @ali5ter.