feat: decouple from ObjectMapper #1953
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-merge-robot
added
the
needs-rebase
| } | ||
|
|
||
|
|
||
| ObjectMapper mapper = new ObjectMapper(); |
There was a problem hiding this comment.
Do I understand correctly that this mapper will be adapted? We will create an instance even if the user provides one I guess.
| } | ||
|
|
||
|
|
||
| ObjectMapper mapper = new ObjectMapper(); |
There was a problem hiding this comment.
Maybe rename it to "DEFAUL_OBJECT_MAPPER", this effectively static.
|
|
||
| // override the configuration service to use the same client | ||
| if (overrider != null) { | ||
| overrider = overrider.andThen(o -> o.withKubernetesClient(this.kubernetesClient)); |
There was a problem hiding this comment.
Would be good to have unit/integration tests for these setting parts. Also shouldn't we deprecate the client as param if it is also in ConfigurationService?
There was a problem hiding this comment.
I don't understand your last point. Regarding unit tests, this is far from finished. I'm not going to move forward with this unless we agree on the approach to take first.
metacosm
force-pushed
the
fix-object-mapper
branch
from
71aca8d to
f26c41d
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
metacosm
force-pushed
the
fix-object-mapper
branch
from
f26c41d to
2a28cc2
Compare
| @@ -261,6 +253,11 @@ static ConfigurationService newOverriddenConfigurationService( | |||
| return baseConfiguration; | |||
| } | |||
|
|
|||
| static ConfigurationService newOverriddenConfigurationService( | |||
There was a problem hiding this comment.
This feels little smelly, since in an abstraction we creating an overrode of specific implementation, is this really needed?
There was a problem hiding this comment.
Not really, just noticing that the overriding method is almost always called passing it a new BaseConfigurationService instance. Also, I think it makes it easier for users because they might not know which instance to use to override the defaults. Open to cleaning this up further.
* refactor: use unmarshal instead of adding new method, works with 6.7.2 * feat: provide and use Kubernetes client directly
* refactor: use unmarshal instead of adding new method, works with 6.7.2 * feat: provide and use Kubernetes client directly
next