-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Allow more characters in graphite tags #9249
Conversation
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
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.
Overall looks good. Just a few comments around different naming for clarity, linter errors and not adding a bool option to allow for future extension.
I think you could link this PR to #8877 also |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
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.
Looks good, thanks for making those changes so quickly!
No problem. I really want this feature, as I'm about to go through a massive new deployment and would love tag values to look right. Any idea what release it will apply to? |
I think it will make it into the next feature release v1.19.0 which will be in June |
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.
Looks good. the regexes are a bit dense but the test coverage looks decent. One comment from me.
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Required for all PRs:
resolves #9248
Added a new function to the Graphite Serializer that will use a specifc sanitzer for tags, along with supporting changes elsewhere.
As it's a relatively breaking change, it's hidden behind a (currently poorly named) feature flag called "graphite_tag_new_sanitize". That name needs to be changed, but I just can't for the life of me think of a better one.