-
Notifications
You must be signed in to change notification settings - Fork 22
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
Disable in-process transport by default and add @AutoConfigureInProcessTransport annotation #88
base: main
Are you sure you want to change the base?
Conversation
…ssTransport annotation
import org.springframework.grpc.test.LocalGrpcPort; | ||
import org.springframework.test.annotation.DirtiesContext; | ||
|
||
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, | ||
properties = { "spring.grpc.server.servlet.enabled=false", "spring.grpc.server.port=0" }) | ||
@AutoConfigureInProcessTransport |
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.
This is wrong? I think we need it to be off in this test.
import org.springframework.grpc.test.LocalGrpcPort; | ||
import org.springframework.test.annotation.DirtiesContext; | ||
|
||
@SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT, properties = "spring.grpc.server.reactive.enabled=false") | ||
@AutoConfigureInProcessTransport |
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.
This is not intended to be an in-process test, I think?
|
||
@Target(ElementType.TYPE) | ||
@Retention(RetentionPolicy.RUNTIME) | ||
public @interface AutoConfigureInProcessTransport { |
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.
Isn't this supposed to be @ImportAutoConfiguration
? Copy the annotations from one of the existing @AutoConfigure*
annotations.
import org.springframework.core.env.ConfigurableEnvironment; | ||
import org.springframework.core.env.MapPropertySource; | ||
|
||
public class InProcessTransportEnvironmentPostProcessor implements EnvironmentPostProcessor { |
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.
I'm not sure this is going to work. Don't we need something that is aware of the test environment? Look at the existing @AutoConfigure*
annotations to see how they are implemented.
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 so much for the tip. I've looked into it more, tested it, and I'd like to say. Could we create an InProcessFactory that will implement ContextCustomizerFactory? That is, make a class like ObservabilityContextCustomizerFactory
Here I have added the
AutoConfigureInProcessTransport
annotation which should be set over the test where we want in-process transport to be enabled. I also added the classInProcessTransportEnvironmentPostProcessor
which implementsEnvironmentPostProcessor
and where I useAnnotationUtils.findAnnotation
to check if the startup class is marked with an annotation or not. If yes, then we need to enable in-process transport and we setspring.grpc.inprocess.enabled=true
. I also setmatchIfMissing
to false inInProcessGrpcServerFactoryAutoConfiguration
(ignored as it is by default) so that in-process will default to false.Closes #77