Skip to content
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

Conversation

matej-g
Copy link
Contributor

@matej-g matej-g commented Jan 19, 2023

Resolves #1352.

This change aims to replace the deprecated gorilla/mux with gin 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 [email protected]

@matej-g matej-g force-pushed the target-allocator-replace-gorilla-dep-with-gin branch from 361e78d to 0931ab5 Compare January 19, 2023 16:13
@matej-g matej-g marked this pull request as ready for review January 19, 2023 16:14
@matej-g matej-g requested review from a team January 19, 2023 16:14
Copy link
Member

@frzifus frzifus left a 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
Copy link
Member

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?

Copy link
Contributor Author

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.

@jaronoff97
Copy link
Contributor

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 :)

@pavolloffay pavolloffay merged commit 072ed8c into open-telemetry:main Jan 26, 2023
iblancasa pushed a commit to iblancasa/opentelemetry-operator that referenced this pull request Jan 31, 2023
…gin` (open-telemetry#1383)

* Replace dependency and adjust HTTP methods and configuration

Signed-off-by: Matej Gera <[email protected]>

* Add changelog

Signed-off-by: Matej Gera <[email protected]>

Signed-off-by: Matej Gera <[email protected]>
iblancasa pushed a commit to iblancasa/opentelemetry-operator that referenced this pull request Feb 1, 2023
…gin` (open-telemetry#1383)

* Replace dependency and adjust HTTP methods and configuration

Signed-off-by: Matej Gera <[email protected]>

* Add changelog

Signed-off-by: Matej Gera <[email protected]>

Signed-off-by: Matej Gera <[email protected]>
@matej-g matej-g mentioned this pull request Feb 8, 2023
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
…gin` (open-telemetry#1383)

* Replace dependency and adjust HTTP methods and configuration

Signed-off-by: Matej Gera <[email protected]>

* Add changelog

Signed-off-by: Matej Gera <[email protected]>

Signed-off-by: Matej Gera <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[target allocator] Replace deprecated gorilla/mux dependency
5 participants