refactoring: remove redundant PlatformAdapter interface #3331
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@metacosm please open an issue and discuss in the public slack channel before opening PRs if possible 🙂 |
|
I wanted to check the tests actually before proposing this as an actual change, hence the draft status… 😉 |
Codecov Report
@@ Coverage Diff @@
## master #3331 +/- ##
==========================================
+ Coverage 45.47% 45.50% +0.02%
==========================================
Files 111 111
Lines 10942 10942
==========================================
+ Hits 4976 4979 +3
+ Misses 5481 5480 -1
+ Partials 485 483 -2
Continue to review full report at Codecov.
|
Test are not automatically executed on Draft PRs ;-) You need to manually trigger them /test all |
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/retest |
|
I would prefer to use the interface as that enforces a layer of abstraction and someday allows someone to mock it for unit tests |
Can you please explain? There's already such an interface: |
| @@ -12,8 +12,8 @@ import ( | |||
| "github.com/openshift/odo/pkg/odo/util/pushtarget" | |||
| ) | |||
|
|
|||
| // NewPlatformAdapter returns a Devfile adapter for the targeted platform | |||
| func NewPlatformAdapter(componentName string, context string, devObj devfileParser.DevfileObj, platformContext interface{}) (PlatformAdapter, error) { | |||
| // NewComponentAdapter returns a Devfile adapter for the targeted platform | |||
There was a problem hiding this comment.
can we also rename the ComponentAdapter interface to PlatformAdapter?
https://github.com/openshift/odo/blob/7da08b69bab0cc6e172c4e1a39ac570566f26d1d/pkg/devfile/adapters/common/interface.go#L4
IMO as this interface is intended for different platform(docker/k8s), PlatformAdapter sounds better, and component term is already too overloaded in odo.
There was a problem hiding this comment.
Agreed, I just kept the most used interface and its name but it's probably indeed better to rename it and maybe move it somewhere where it makes more sense.
There was a problem hiding this comment.
Actually, after taking another look at the code, ComponentAdapter is appropriate because its API deals with component and adapts the underlying implementation to the component functionality so I think it's OK to keep ComponentAdapter.
There was a problem hiding this comment.
but there is a component adapter present here https://github.com/openshift/odo/blob/5f71123d4332e47116b8c8112e76f91ec73f8833/pkg/devfile/adapters/kubernetes/component/adapter.go#L29
its not called componentAdapter but its inside component/adapter.go wouldn't that be confusing?
There was a problem hiding this comment.
There's also one here… What's your point? I didn't do an indepth refactoring here, just a quick win.
|
/retest |
|
/approve |
openshift-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: girishramnani The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
What type of PR is this?
/kind cleanup
What does does this PR do / why we need it:
Remove seemingly redundant PlatformAdapter interface
Which issue(s) this PR fixes:
Fixes #3333
How to test changes / Special notes to the reviewer:
Behavior shouldn't be impacted.