-
Notifications
You must be signed in to change notification settings - Fork 363
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
pkg/installation: cleanup refactoring #296
Conversation
- `getDownloadTarget()`: removed, it didn't do anything interesting. - `downloadAndMove()`: - renamed to `downloadAndExtract()` which better reflects the functionality - reduced methods from 7 to 4 - refactored the "move" operation out of this method (decouples "move" from "download+extract") and we now call "move" from install(). - added unit tests (both for downloading a file, and --archive override) - introduced `installOperation` which contains the minimal information to perform installation operation. - introduced `InstallOpts` type to scale out (in the future) - `install()`: - accepts fewer arguments (2) than before (8) - no longer knows about index.Plugin, or environment.Paths - cmd/install: removed a redundant check detecting specifying plugin names along with --manifest option. Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Codecov Report
@@ Coverage Diff @@
## master #296 +/- ##
==========================================
- Coverage 56.22% 55.04% -1.18%
==========================================
Files 19 19
Lines 891 901 +10
==========================================
- Hits 501 496 -5
- Misses 337 353 +16
+ Partials 53 52 -1
Continue to review full report at Codecov.
|
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 work. This is much cleaner now.
Finally install
becomes somewhat testable. This could be one of the next steps. For that we just need to override downloadAndExtract
or mock the http server as you already did.
return errors.Wrap(err, "failed while moving files to the installation directory") | ||
} | ||
|
||
subPathAbs, err := filepath.Abs(op.installDir) |
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.
Could you extract the whole sub-path checking logic to a separate function?
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 we do that in another PR? 😄
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.
sure :)
Yeah, I think |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, corneliusweig 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 |
getDownloadTarget()
: removed, it didn't do anything interesting.downloadAndMove()
:downloadAndExtract()
which better reflects the functionalityfrom "download+extract") and we now call "move" from install().
introduced
installOperation
which contains the minimal information toperform installation operation.
introduced
InstallOpts
type to scale out (in the future)install()
:cmd/install: removed a redundant check detecting specifying plugin names
along with --manifest option.
Fixes #293
/kind cleanup
/assign @corneliusweig