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.
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.
So the trouble I have with this change is that it muddies the waters... What if it's to be an expression now? Like
http.response_code - 12
? The white spaces matter now?That being said I don't think we'd ever support that (very example if only 🤣 ). That being said I already hated having the
.
in there... tho I thought you could have argued it remained an identifier for "something" to resolve...So anyways, I won't necessarily block this from happening, but I wonder if sanitizing the variable names/identifiers wouldn't be slightly more desirable.
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.
We don’t necessarily need
-
nor/
. Intuitively, we went with these characters because the Limitador identifiers that the RateLimitPolicy controller generates come straight from qualified limit definitions stated in Kubernetes resources, where the/
char is used as the namespace-resource separator and the-
char is often used in the names of the resources. We first thought that by keeping such format we'd favour readability (even though we’re here talking about internal values of internal resources that hardly will be inspected by humans).In the end, all we need are unique limit identifiers. Readability and easy referencing of those bits of configuration across resources are a plus. In fact, while we are using the namespaced name of the RateLimitPolicy resource in the
namespace
field of the rate limit definition, only the name of the limit for this particular identifier suffices. Replacing the unsupported characters there won't be a problem.All that to say that we don't need this PR. We can fix some other way in the RateLimitPolicy controller, in https://github.com/kuadrant/kuadrant-operator.
Thanks @didierofrivia and @alexsnaps! I think we can close this.