Config simplifications #1096
Config simplifications #1096
Conversation
`ErrorConfiguration` is already found during component-scanning of `com.netflix.spinnaker.config`.
- Use parameter injection for config properties that are still used - Delete the unused properties that were injected - Use a `Jackson2ObjectMapperBuilder` bean to simplify setup of the default `ObjectMapper`
- Implement `WebMvcConfigurer` instead of extending the now deprecated `WebMvcConfigurerAdapter` class - Use `List::of` instead of `ImmutableList::of` - Remove call to `ContentNegotiationConfigurer::favorPathExtension` as this value has been the default in Spring for a while now - Replace `val` with `var` keyword - Fix some grammar in docs
Use `ServiceClientProvider` from `kork-web` to create Retrofit proxy beans for `Front50Api`, `ClouddriverApi`, and `IgorApi`. Other Retrofit settings repeatedly configured here are already configured in the relevant `ServiceClientFactory` ultimately used by `ServiceClientProvider`.
- Use `ServiceClientProvider` from `kork-web` to create a Retrofit proxy bean for `FiatService` - Use `Jackson2ObjectMapperBuilder` to create the `ObjectMapper` used for `FiatService` - Use `withDefaults()` customizer to simplify security configuration chains
fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/RetrofitConfig.java
Show resolved
Hide resolved
fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/RetrofitConfig.java
Show resolved
Hide resolved
fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/FiatConfig.java
Show resolved
Hide resolved
| .build() | ||
| .create(Front50Api.class); | ||
| Front50Api front50Api(@Value("${services.front50.base-url}") String front50Endpoint) { | ||
| return provider.getService( |
There was a problem hiding this comment.
I believe this ends up with RetrofitServiceFactory creating the returned object. At the moment that code doesn't use
.setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
so this ends up being a change in behavior. Adding the setErrorHandler call to RetrofitServiceFactory is in the plans, but is gonna take some care (and potentially a config flag controlling whether it happens or not) since I believe both gate and echo use it, and we need to coordinate changes in handling of RetrofitError to Spinnaker*Exception.
There was a problem hiding this comment.
Isn't the new error handler for running in Retrofit2? Right now, ServiceClientFactory implementations check for the use of the Call<T> API for determining which Retrofit version to use.
There was a problem hiding this comment.
SpinnakerRetrofitErrorHandler is for retrofit1.
There was a problem hiding this comment.
But Retrofit only throws RetrofitError in the first place?
There was a problem hiding this comment.
Yup, retrofit1 throws RetrofitError, and SpinnakerRetrofitErrorHandler turns RetrofitError into SpinnakerServerException (and children).
There was a problem hiding this comment.
Oh, so the point of the error handler Bean is to translate the retrofit exception into a spinnaker one?
There was a problem hiding this comment.
It's not a bean, but yes.
Provided in multiple commits, I've modernized and simplified several config classes by reusing Kork and Spring Boot things.