Add support for using cluster name for the delete cluster command

[axbarsan]

Towards https://github.com/giantswarm/gsctl/issues/188

At the moment, this only provides support for the delete cluster command

• Renamed ClusterIDMissing errors to ClusterNameOrIDMissing
• Before executing the delete request, it gets all the clusters and checks to see if there is a cluster with the provided ID or name in there
• Changed error messages to also say cluster name, not just ID

When a cluster is deleted, the confirmation will state the name/ID provided by the user. So if the user provided an ID, the confirmation will show the ID; or a name, if he provided a name.

Preview

• Deleting a cluster with the name
  

• Deleting a cluster with the ID
  

• Trying to delete a cluster with an ID/name that doesn't exist
  

• Trying to delete a cluster that has the same name as another existing one
  

• Saving cluster IDs in the cache file (~/.config/gsctl/clustercache.yaml)
  

As a side note, this only includes the delete cluster command, because I want to know that my implementation is right, before copying and pasting it to the other commands.
Also, I would like to do 1-2 commands per PR, to keep them small, so we could prevent any unwanted bugs.

[ubergesundheit]

Cool!

[oponder]

See my comment on the name ambiguity edge case. Might need a bit of UX consideration in how we present that message.

Like the coolest would be to offer some kind of picker that is like

Found more than one cluster called "My Cluster", please type the id of the cluster that you would like to delete:

CLUSTER ID    ORGANIZATION    NAME
123ab         testing         My Cluster 
456cd         production      My Cluster 

But if that needs more UX consideration, then for now just failing and asking the user to use a cluster ID instead would be ok with me.

[axbarsan]

See my comment on the name ambiguity edge case. Might need a bit of UX consideration in how we present that message.

Like the coolest would be to offer some kind of picker that is like

Found more than one cluster called "My Cluster", please type the id of the cluster that you would like to delete:

CLUSTER ID    ORGANIZATION    NAME
123ab         testing         My Cluster 
456cd         production      My Cluster 

But if that needs more UX consideration, then for now just failing and asking the user to use a cluster ID instead would be ok with me.

I like that, I'll try to implement it

[marians]

So my understanding is we basically have to list clusters (read: GET /v4/clusters/) in every command that needs a cluster ID argument currently, before we can issue the API call. Because we don't know in an input like

gsctl create kubeconfig -c a1b2c

whether a1b2c is a cluster ID or cluster name.

In installations with many organizations and clusters we see this taking several seconds (in one case, more than 5). So every command would get delayed by that duration. To me, that is too much of a downside.

What are other ways we can think of? Some ideas:

• Maintain a cache of last seen cluster IDs: If input a1b2c is a hit in our cache, we assume that it's an ID and don't try to resolve name to ID first. (We could re-build that cache periodically and/or whenever we call GET /v4/clusters/)
• Allow users to turn off the feature? When turned off, the behaviour should be just like it is currently.

[axbarsan]

Yes, we do fetch all clusters in every command.

I believe caching wouldn't be an ideal solution, especially if we encounter situations like this:

• Creating a cluster in gsctl
• Deleting it through happa or direct API call and creating a new one with the same name
• Deleting it in gsct would throw an error, because in the cache there's another ID stored for that name, and the API will error out, saying it does not exist

Allow users to turn off the feature? When turned off, the behaviour should be just like it is currently.
I would vote for this

Also, would it make sense to have a -n|--name param that will specify the name? So it will only make an extra API call if you do it that way

[marians]

The caching idea was not about caching cluster names. Rather only cache cluster IDs we really got from the API. So every hit on that cache could be considered a valid cluster ID that at least has existed at some point.

[axbarsan]

Oh I get it now, so we'll cache the cluster IDs, so if we provided an ID, it would check the cache and if it's there, it would not make the API call. That sounds good, I can start on that

[axbarsan]

I added support for name collisions, which also prints a table with the colliding clusters, and then you can enter the ID of the cluster that you want to use.

Updated the description with the preview.

Next: Adding the ID cache

[coveralls]

Coverage increased (+0.6%) to 46.885% when pulling 4a2b7d5 on add-support-for-using-cluster-name into 8445555 on master.

[axbarsan]

Also added caching and unit tests for caching. Right now, cluster IDs are saved in the cache for every list clusters execution, and also, for using the GetClusterID() function, it will cache the IDs of all the clusters found.

[marians]

After I opened the util package, I learned that it isn't good Go practice to have such a catch-all package. In Go, devs expect explanatory package names. Look at all the folders on the top level. It would fit the current layout of the repo to just add a package like cache or, more descriptive, cluster-cache.

[marians]

I haven't looked at everything yet, so might have missed it. One more questions regarding the cluster cache: Does the cache ever expire? Would cluster IDs get re-fetched when the cache is older than a certain duration?

One thing to keep in mind is that the cache is not endpoint/installation specific. So what is a valid ID in one installation might not be one in another. But with cluster IDs as random as they are, I don't see that as a problem.

[axbarsan]

Thank you for the thorough review, @marians . I really appreciate the suggestions you've made.

I didn't take into consideration expiry dates and endpoint-specific IDs, but I think it may be worth switching to a YAML structure to introduce those, to make it adding other functionality easier to do in the future

[axbarsan]

• Moved all cluster cache utils to the clustercache package
• Made the cache save to a YAML file
• Added different IDs per API endpoint
• Added expiration dates for cluster caches

Updated the description with a preview of the file

[marians]

Nitpick: Some people in the company will be much happier to find sentences in comments ending in a full stop. Do as you wish.

[marians]

We spoke about a little UX improvement to be made when the name is ambiguous.

Besides that, great work! ❤️

[axbarsan]

• Removed the Name column from the name collision table.
• Added Release and Created columns instead.
• Modified the messages printed if there are name collisions

Updated the description with the preview