UpCloudLtd / upcloud-cli

UpCloud command line client (upctl)
https://upcloudltd.github.io/upcloud-cli/
MIT License
30 stars 9 forks source link

fix(matchers): name matching case-insensitive #335

Open MarinX opened 1 month ago

MarinX commented 1 month ago

Fixes #102

Added strings.EqualFold to MatchArgWithWhitespace function name. Added tests for matchers.go file.

./upctl network list                  

 UUID                                   Name              Router   Type      Zone    
────────────────────────────────────── ───────────────── ──────── ───────── ─────────
 0365d711-ab40-425e-bab3-92a1e351996d   example network            private   de-fra1 
./upctl network show "Example Network"

  Common
    UUID:   0365d711-ab40-425e-bab3-92a1e351996d 
    Name:   example network                      
    Router:                                      
    Type:   private                              
    Zone:   de-fra1                              

  Labels:

    No labels defined for this resource.

  IP Networks:

     Address       Family   DHCP   DHCP Def Router   DHCP DNS 
    ───────────── ──────── ────── ───────────────── ──────────
     10.0.0.0/24   IPv4     yes    no                         

  Servers:

     UUID                                   Title                                       Hostname             State   
    ────────────────────────────────────── ─────────────────────────────────────────── ──────────────────── ─────────
     0088f557-7939-48ec-b589-6138dbd4f084   ubuntu.example.tld (managed by terraform)   ubuntu.example.tld   started 
kangasta commented 3 weeks ago

Thank you for the pull request! Based on quick testing in a terminal, this seems to fix the resolving part of #102. However, this would now be a breaking change for users that have resources named so that there is difference only in letter case (e.g., McDuck and mcduck), but that should be quite unlikely.

upctl router create --name mcduck
upctl router create --name McDuck
upctl router show  McDuck
# Succeeds with case-sensitive matching, fails with case-insensitive matching 

One option would be to use case-sensitive match in this case, but not sure how big change that would require in the resolver that calls the MatchArgWithWhitespace function 🤔

MarinX commented 3 weeks ago

You are correct that this is breaking change. Especially for users with the same names but different casing. (unlikely but potential bug here) What about adding a --insensitive flag?

upctl router show --insensitive mcduck

So, without a flag, the show command will function as is, while adding --insensitive, will trigger matching words without casing. But, this is related only to insensitive names and its purpose is only 1 thing.

Another option is to maybe add regex to the show command. Something like this

upctl router show --regex mc[a-z]+

Where we can call the regex and return the first match. Maybe I am overengineer this too much, but thinking out loud.

Let me know your thoughts or if you have some other ideas we can discuss :)

kangasta commented 4 days ago

Sorry about the delay with this. I'll try to continue looking into this during next week.