-
Notifications
You must be signed in to change notification settings - Fork 440
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
[target-allocator] Replace deprecated gorilla/mux
dependency with gin
#1383
[target-allocator] Replace deprecated gorilla/mux
dependency with gin
#1383
Conversation
Signed-off-by: Matej Gera <matej.gera@coralogix.com>
Signed-off-by: Matej Gera <matej.gera@coralogix.com>
361e78d
to
0931ab5
Compare
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.
Thanks @matej-g for taking care 👍 Actually i never used gin and the added context, feels a bit strage - but aside of that it looks good to me. :)
Whats your thoughts @open-telemetry/operator-ta-maintainers ?
|
||
router := gin.Default() | ||
router.UseRawPath = true | ||
router.UnescapePathValues = false |
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.
Why is it explicit set to false?
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 is set to true
by default so I'm setting it to false
explicitly here. Basically these two options should provide same functionality as we had with gorilla/mux
and UseEncodedPath
.
If it helps I can add a comment, although if you consult the comments for the gin.Engine
type it should be clearer.
I'm rerunning the unit tests with @kristinapathak 's recent change here just to be sure we have some added confidence on changing the framework :) |
…gin` (open-telemetry#1383) * Replace dependency and adjust HTTP methods and configuration Signed-off-by: Matej Gera <matej.gera@coralogix.com> * Add changelog Signed-off-by: Matej Gera <matej.gera@coralogix.com> Signed-off-by: Matej Gera <matej.gera@coralogix.com>
…gin` (open-telemetry#1383) * Replace dependency and adjust HTTP methods and configuration Signed-off-by: Matej Gera <matej.gera@coralogix.com> * Add changelog Signed-off-by: Matej Gera <matej.gera@coralogix.com> Signed-off-by: Matej Gera <matej.gera@coralogix.com>
…gin` (open-telemetry#1383) * Replace dependency and adjust HTTP methods and configuration Signed-off-by: Matej Gera <matej.gera@coralogix.com> * Add changelog Signed-off-by: Matej Gera <matej.gera@coralogix.com> Signed-off-by: Matej Gera <matej.gera@coralogix.com>
Resolves #1352.
This change aims to replace the deprecated
gorilla/mux
withgin
while maintaining the functionality of current solution.Note: Due to difference in convention for path variables used by both frameworks (
gorilla/mux
using curly braces, .e.g/jobs/{job_id}/targets
vs.gin
using colon e.g./jobs/:job_id/targets
), the only change will be in the reported metric label for the jobs path.Signed-off-by: Matej Gera matej.gera@coralogix.com