Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
rds: allow case_insensitive path matching #3997
rds: allow case_insensitive path matching #3997
Changes from all commits
00069bb
18d454e
287618c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also:
fullPath: "/s/M", path: "/S/m"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar:
prefix: "/S/", path: "/s/m"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm ... does it make sense for our internal types to match the xDS proto in the sense that we also store whether the match is
CaseSensitive
instead of the other way around?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used case insensitive in the field because the default value of bool is false, and it's a sane default.
If I make it case sensitive, I will need to make it a
*bool
, so when it's not set,nil
means case sensitive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to me with the way the proto is structured that they want the default to be case sensitive. This is comment on the field:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If nothing is configured for case-sensitive-or-not, matching should be
case-sensitive == true
.If the field is
CaseSensitive
, it has to be a*bool
, so if nothing is configured, its default value isnil
, and we can handle it ascase-sensitive == true
If the field is
CaseInsensitive
, when nothing is configured, it's default value isfalse
, same ascase-sensitive == true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the comment on the proto field, if the field is not set, we should assume a value of
true
, right?Hmm ... I guess that's what we end up with because of the double negation with the
caseInsensitive
. If we can always set the value of our field correctly here, wouldn't it be better to use a field which matches the proto?I just feel that this inconsistency might bite us later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we really want to be consistent, the CaseSensitive field has to be a boolean pointer. I really don't like using pointers when we can have a default value that just works, because you will need to check nil before reading its value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really suggesting that we use a
*bool
. But, given that when we process the received route configuration, we are anyways looking to see whether this field is set or not? So, we could still use acaseSensitive bool
and set it up as we want based on whether this field exists, and if it exists, based on its value.But if you are really happy with the way things are, then that's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm keeping it for now. If we later find this is causing problems, we can update. This is not a public API.