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 #1352

Closed
matej-g opened this issue Jan 6, 2023 · 10 comments · Fixed by #1383
Closed

[target allocator] Replace deprecated gorilla/mux dependency #1352

matej-g opened this issue Jan 6, 2023 · 10 comments · Fixed by #1383
Labels
area:target-allocator Issues for target-allocator help wanted Extra attention is needed

Comments

@matej-g
Copy link
Contributor

matej-g commented Jan 6, 2023

The allocator server uses the router from gorilla/mux, however the project has been deprecated recently (https://github.com/gorilla#gorilla-toolkit) and is no longer actively maintained. We should replace it with an alternative that's still maintained.

@matej-g
Copy link
Contributor Author

matej-g commented Jan 6, 2023

(also if there's agreement on this, happy to take this / propose and replace with an alternative 🙃)

@pavolloffay pavolloffay added area:target-allocator Issues for target-allocator help wanted Extra attention is needed labels Jan 6, 2023
@pavolloffay
Copy link
Member

cc) @jaronoff97

@matej-g thanks for reporting this. Do you have any good alternatives in mind?

@matej-g
Copy link
Contributor Author

matej-g commented Jan 6, 2023

Not sure what else is commonly used within Go projects these days, in other projects I'm involved in we use https://github.com/go-chi/chi and we had good experience with it.

@frzifus
Copy link
Member

frzifus commented Jan 6, 2023

seems that only effects the otel-allocator.

@jaronoff97
Copy link
Contributor

@matej-g the other framework I was considering was gin is there a reason to use chi over gin? I don't have a preference FWIW, just want to add another option in to the mix.

@frzifus
Copy link
Member

frzifus commented Jan 9, 2023

Is there any specific need to such a framework? As far as I can see, the only functionality that is not in the std lib is the query of the url path variable {job_id}.

@matej-g
Copy link
Contributor Author

matej-g commented Jan 10, 2023

Hm that's also a good point @frzifus, I think I can try to prepare a PR with either alternative (standard lib or one of the frameworks if we need the extra features) and we could continue from there.

@jaronoff97
Copy link
Contributor

@matej-g that would be great, thank you! My worry with the standard library is that if we need to use features that something like gin or chi provide how hard it would be to switch? If it's low enough and we can make it work with the stdlib, then let's go that route. Either way having a PR to compare would be super helpful. Thank you again @matej-g.

@matej-g
Copy link
Contributor Author

matej-g commented Jan 19, 2023

👋 I prepared PR replacing gorilla/mux with gin (#1383), after all due to the need to handle the variable in the path as @frzifus mentioned, there would be more added complexed if we were to coded that by hand, so I believe using gin is a good alternative. Feedback welcome!

@frzifus
Copy link
Member

frzifus commented Jan 19, 2023

Awesome, I will have a look soon. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:target-allocator Issues for target-allocator help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants