-
Notifications
You must be signed in to change notification settings - Fork 243
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
Refacto odo dev #6141
Refacto odo dev #6141
Conversation
@feloy: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
✅ Deploy Preview for odo-docusaurus-preview ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Nice refactor!
How do we know that the PR fixes this issue? Have we tried this with podman? |
If you look at the code into pkg/odo/cli/dev, you should not see any code related to Kubernetes (1). Also, into the interface definition in pkg/dev, no parameter of the methods should be related to Kubernetes. (1) There is still a problem about calling:
but this is a generic problem how we are managing the context, not specific to the |
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.
LGTM - thanks for this!
Added a few nitpicky comments.
pkg/dev/dev.go
Outdated
"io" | ||
|
||
"github.com/redhat-developer/odo/pkg/binding" | ||
"github.com/redhat-developer/odo/pkg/envinfo" | ||
"github.com/redhat-developer/odo/pkg/kclient" | ||
"github.com/redhat-developer/odo/pkg/portForward" | ||
"github.com/redhat-developer/odo/pkg/preference" | ||
"github.com/redhat-developer/odo/pkg/testingutil/filesystem" | ||
filesystem "github.com/redhat-developer/odo/pkg/testingutil/filesystem" |
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.
nit: is this import alias needed? I find it a bit redundant.
https://github.com/redhat-developer/odo/wiki/Dev:-Coding-Conventions#import-aliasing
pkg/dev/interface.go
Outdated
|
||
// Watch watches for any changes to the files under path while ignoring the files/directories in ignorePaths. | ||
// | ||
// It then watches for any changes to the files under path while ignoring the files/directories in ignorePaths. | ||
// It logs messages to out and uses the Handler h to perform push operation when anything changes in path. | ||
// It uses devfileObj to notify user to restart odo dev if they change endpoint information in the devfile. | ||
// If debug is true, the debug command will be started after a sync, or the run command by default. | ||
// If buildCommand is set, this will look up the specified build command in the Devfile. Otherwise, it uses the default one. | ||
// If runCommand is set, this will look up the specified run command in the Devfile and execute it. Otherwise, it uses the default one. | ||
// componentStatus is the status returned from the call to the Start Method |
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.
Can you remove the doc about componentStatus
since it is not a parameter of Start
?
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 have also rewritten the doc
Kudos, SonarCloud Quality Gate passed!
|
Thanks for the discussion and the work @feloy. /lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rm3l 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 |
/override ci/prow/v4.11-integration-e2e Tests pass on IBM Cloud |
@feloy: Overrode contexts on behalf of feloy: ci/prow/v4.11-integration-e2e In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/override ci/prow/v4.11-integration-e2e |
@feloy: Overrode contexts on behalf of feloy: ci/prow/v4.11-integration-e2e In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
* Move Dev Handler to specific file * Do not pass unnecessary namespace to Dev package * Pass FS with dependency injection * Merge Dev.Start and Dev.Watch * Simplify Handler * Move Dev Handler inside the Dev package * Update mock * Review
What type of PR is this:
/kind cleanup
What does this PR do / why we need it:
Which issue(s) this PR fixes:
Part of #5865
Fixes #6159
PR acceptance criteria:
Unit test
Integration test
Documentation
How to test changes / Special notes to the reviewer: