Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Nov 18, 2024
Merged

Conversation

MarinX
Copy link
Contributor

@MarinX MarinX commented Oct 4, 2024

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 

@MarinX MarinX requested a review from a team as a code owner October 4, 2024 18:40
@kangasta
Copy link
Contributor

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
Copy link
Contributor Author

MarinX commented Oct 21, 2024

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
Copy link
Contributor

kangasta commented Nov 8, 2024

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

@kangasta
Copy link
Contributor

I did some testing on how the current resolver logic could be refactored to support multiple matching levels in #341. That PR adds support for using UUID prefix as argument (similarly than in e.g. git and docker commands) and the case-sensitive matching implemented in this PR could work in similar way. I'll discuss those changes with my colleague and let's merge this PR in after #341 has been merged to main.

@kangasta
Copy link
Contributor

#341 has now been merged, I'll rebase this on top of main early next week and will after that squash and merge this into main.

@kangasta kangasta merged commit 3e19805 into UpCloudLtd:main Nov 18, 2024
6 of 7 checks passed
@kangasta
Copy link
Contributor

Thank you for the contribution @MarinX! 👍 This is now released in v3.12.0 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Case-sensitive resource matching
2 participants