tiltfile: object identifiers are not case sensitive#3429
Conversation
| } | ||
|
|
||
| for i, o := range opts.objects { | ||
| entities, ok := fragmentsToEntities[o] |
There was a problem hiding this comment.
i'm surprised they're not stored in objects as lowercase?
There was a problem hiding this comment.
Hmm it might be weird to use a different string than the one the user entered in error messages though, even if we treat it as case insensitive. I think I want to preserve the original string.
| exactOrEmptyRegex(name), | ||
| exactOrEmptyRegex(namespace), | ||
| ) | ||
| func newExactCaseInsensitiveK8sObjectSelector(apiVersion string, kind string, name string, namespace string) (k8sObjectSelector, error) { |
There was a problem hiding this comment.
I feel like the nomenclature here needs to be cleaned up a bit, in particular:
- All functions are case insensitive now, but only one of them has "CaseInsensitive" in the name. That seems weird?
- The usage of the word "exact" here is not something i've seen before. I think python calls this "FullMatch"?
There was a problem hiding this comment.
ooh I like FullMatch. And I agree, I'll clean this stuff up.
| return ret, nil | ||
| } | ||
|
|
||
| func makeCaseInsensitive(s string) string { |
There was a problem hiding this comment.
i feel like half the problem here is that we're passing around a regexp around as a DSL string, rather than something more structured, and i wonder if this code would be easier to follow if we had something like
type re struct {
pattern string
ignoreCase bool
fullMatch bool
}
func (r re) compile() { ... }
There was a problem hiding this comment.
Yeahhh I tried something like this back when I was originally writing this a couple weeks ago. I'll give it another shot with fresh eyes.
|
Did you end up finding a source or some evidence for whether k8s allows two objects to have the same name with different casing? It's probably worth writing down the evidence/assumption, given we were unclear on it. |
|
Here are some references: As a general rule of thumb, Kubernetes tends to convert everything to lowercase, since they end up getting converted to case-insensitive HTTP endpoints |
|
Yeah that stuff is what I went off of. I'm still a little unclear on kind names, but it seems that most everything has been lowercase in my testing. Like in this part of the docs
It doesn't explicitly say that kind names are case insensitive, but I wasn't able to test this myself yet. |
|
Ya, I think the API docs are being super-precise in a way that's helpful if you understand the internals, but hard to parse if you do not. There are two different things:
and there's a mapping from Kind -> resource that converts to lowercase but to a user (e.g., someone who's just using kubectl and writing yaml), because there's a 1<>1 mapping, it's easy to read that as "oh, it's case-insensitive", when what's actually happening is that you're interacting with different things and it's not always obvious to you which one you're interacting with Does that make sense? |
|
Yeahhh that makes sense. Thanks @nicks! |
|
I updated some nomenclature but left the refactor as a TODO because, once again, I ran in to weird test failures. I think we're depending on some regex behavior that is unclear to me and super finicky. |
3 participants
Fixes #3423