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

fix: restore deprecated interceptor modules #292

Merged

Conversation

wingyplus
Copy link
Contributor

@wingyplus wingyplus commented Jan 11, 2023

I found the compilation warning in interop tests since PR #289 was merged. Because of GRPC Prometheus didn't change to use the new interceptor module. And it's suddenly breaking changes. So I propose to bring it back but add the deprecated module to it. The rest topic is discussing when to remove it. :)

Since PR elixir-grpc#289, the `GRPC.ServerInterceptor` and
`GRPC.ClientInterceptor` is suddenly remove. This could make breaking
changes. This changes restore it and add deprecated to `@moduledoc` to
those interceptors.
@polvalente
Copy link
Contributor

Although this makes sense, I'm not sure if the overhead of maintaining both versions is worth it.
I think a PR for grpc-prometheus updating it instead is the better approach.

@wingyplus
Copy link
Contributor Author

Unfortunately. The upcoming version would break my codebase if this PR is not applied. But it's easy to change. :)

For the grpc prometheus, we may need this library to release before changing it since it's an external library.

@polvalente
Copy link
Contributor

We'll probably deprecate the Prometheus library :)

I'll merge this. I think it's nice to warn for at least 1 version on the deprecation.

@polvalente polvalente merged commit bf2ad0e into elixir-grpc:master Jan 11, 2023
@wingyplus
Copy link
Contributor Author

We'll probably deprecate the Prometheus library :)

I'll merge this. I think it's nice to warn for at least 1 version on the deprecation.

Agreed. :)

@wingyplus wingyplus deleted the restore-interceptor-deprecated branch January 11, 2023 16:12
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.

3 participants