Skip to content
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

Save install receipt #195

Merged
merged 14 commits into from Jul 2, 2019

Conversation

Projects
None yet
4 participants
@corneliusweig
Copy link
Contributor

commented Jun 2, 2019

I want to add more tests, however I would ask for some early feedback whether you like the direction this is taking.

Summary of changes

  • There is a new manifest kind "Receipt" which contains the details of the install, such as
    • date of first install
    • date of last update
    • plugin manifest of last install/update
  • The krew info command now looks for index manifests, and receipts. If a receipt is found, the shown information is slightly augmented.
  • plugin instances are now passed around as pointers instead of full copies. Was there a particular reason for not using pointers?
  • a few code movements were necessary to remove import cycles
  • a new dependency on google/go-cmp was added and vendored. If you don't like this, I can also do this manually, but the test assertions won't be so pretty. Otherwise, this library should be used in a few more places.

Todos

There are some loose ends that should probably be taken up in other PRs, because this one is already quite large:

  • maybe add some migration logic, so that receipts are installed for all currently installed plugins (probably as a post-upgrade activity, so that it is guaranteed that the receipts are correct)
  • switch uninstall logic to use the receipts instead of the current index

Close #163

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

/assign @ahmetb

@codecov-io

This comment has been minimized.

Copy link

commented Jun 2, 2019

Codecov Report

Merging #195 into master will decrease coverage by 0.21%.
The diff coverage is 46.34%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #195      +/-   ##
=========================================
- Coverage   53.82%   53.6%   -0.22%     
=========================================
  Files          16      16              
  Lines         732     735       +3     
=========================================
  Hits          394     394              
- Misses        286     289       +3     
  Partials       52      52
Impacted Files Coverage Δ
pkg/installation/install.go 32% <0%> (ø) ⬆️
pkg/installation/upgrade.go 0% <0%> (ø) ⬆️
pkg/environment/environment.go 79.16% <100%> (ø) ⬆️
pkg/info/info.go 100% <100%> (ø) ⬆️
pkg/index/indexscanner/scanner.go 67.34% <50%> (ø) ⬆️
pkg/receipt/receipt.go 66.66% <66.66%> (ø) ⬆️
cmd/validate-krew-manifest/main.go 58.33% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b0255c...ff5b7e6. Read the comment docs.

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

This is a massive PR.
Please don’t expect me to review this very soon. I don’t think we discussed how the implementation would look like, so I have to find time to study it from the PR.

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

But I’m glad we’ll do this before implementing multi-indexes which would definitely require this change.

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Take your time. As said elsewhere, don't hesitate to request parts being split out if you think it doesn't fit here.
BTW, I think it looks larger than it actually is due to the addition of a new dependency.

@corneliusweig corneliusweig force-pushed the corneliusweig:install-receipt branch from 7e42a3b to bb0f363 Jun 5, 2019

@corneliusweig corneliusweig force-pushed the corneliusweig:install-receipt branch from bb0f363 to 42d601c Jun 10, 2019

ahmetb added a commit to ahmetb/krew that referenced this pull request Jun 14, 2019

Make corneliusweig a maintainer
Adding corneliusweig to owners. He has been consistently helping both with
krew and krew-index repositories in terms of:
- developing plugins himself
- taking a stab at krew machinery with large scale code refactors
- adding integration test suite to the project
- adding more validation and test cases
- increasing developer documentation

Some of his notable work:
- kubernetes-sigs#195
- kubernetes-sigs#183
- kubernetes-sigs#191
- kubernetes-sigs#201
- kubernetes-sigs#202
- kubernetes-sigs#203
- kubernetes-sigs#208

He is familiar with the codebase enough to officially review and approve code.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>

ahmetb added a commit that referenced this pull request Jun 18, 2019

Make corneliusweig a maintainer (#214)
Adding corneliusweig to owners. He has been consistently helping both with
krew and krew-index repositories in terms of:
- developing plugins himself
- taking a stab at krew machinery with large scale code refactors
- adding integration test suite to the project
- adding more validation and test cases
- increasing developer documentation

Some of his notable work:
- #195
- #183
- #191
- #201
- #202
- #203
- #208

He is familiar with the codebase enough to officially review and approve code.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

This is still on my list, I'll get to reviewing hopefully soon.

@corneliusweig corneliusweig force-pushed the corneliusweig:install-receipt branch from 42d601c to 89486b3 Jun 18, 2019

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

Before reading the code, I'd like to discuss some points (and this is precisely why we should design it before coding it):

  • There is a new manifest kind "Receipt" which contains the details of the install, such as
    • date of first install
    • date of last update
    • plugin manifest of last install/update

Why do we need a new kind? Can't we just copy the manifest over? (This simplifies a lot.) I don't think there's any point of storing these dates.

  • The krew info command now looks for index manifests, and receipts. If a receipt is found, the shown information is slightly augmented.

I don't know if users can understand this easily. This is a bit "magic" behavior, and will get more complicated when we support multiple indexes and there's name collisions. We should probably look at what other package managers do to show "more info" about a package, and does the behavior change when the package is installed.

  • plugin instances are now passed around as pointers instead of full copies. Was there a particular reason for not using pointers?

I think mostly because there aren't a lot of good reasons to use pointers.

  • We aren't worried about little memory overhead
  • copying reduces the effects of accidentally modifying the Plugin struct instance in a particular function
  • there's no way to get panics due to nil, if it's not a pointer.

So it doesn't add anything, consider undoing this.

  • a new dependency on google/go-cmp was added and vendored. If you don't like this, I can also do this manually, but the test assertions won't be so pretty. Otherwise, this library should be used in a few more places.

I think new test dependencies are ok, our tests have been pretty lean so far, but we can change that.

@corneliusweig corneliusweig referenced this pull request Jun 19, 2019

Closed

REQUEST: New membership for corneliusweig #929

5 of 6 tasks complete
@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

First of all, thanks for your feedback. I think it's absolutely necessary to do that even if there is a working first revistion.
So there seem to be 3 areas that need to be addressed:

  1. Receipt type
  2. passing plugins as object vs. pointer to object
  3. test dependencies
  4. magic krew info

  1. does not need more discussion.
  2. From what I see in other k8s code, it is much more common to pass pointers to objects. However, this is independent of the main point of this PR. So I'll open an issue where we can discuss the pros and cons and drop the respective commit from here.

So I'll only focus on point 1 and 4:

1

The main reason why I introduced the new type was that it gives us more flexibility in the future. For example, when we have multiple indices and somebody introduces a name clash, we can record what the source index for the specific plugin was. The dates which I put into the receipt right now is just a nice-to-have but definitely not important. And there are for sure other important use-cases for saving metadata about the install process in the receipt, which we can't think of right now.
So I'm a bit torn apart between ease of maintenance (no extra type) and flexibility for future use-cases (extra type). I do admit that a single type simplifies the logic somewhat.

4

I don't agree that this appears magic to users. It should be quite obvious that info does not print the install date for a plugin which is not installed. On the other hand, it makes perfect sense to show that for an installed plugin. However, it may be a good idea to show all possible output fields and leave them blank if they don't apply.

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

I do admit that a single type simplifies the logic somewhat.

I'd like to hammer on this, it really does simplify, and I don't think there's much value of showing dates of installation. Most other package managers don't do this either.

The TOP reason to save install receipt is the ability to uninstall plugins that don't exit/never existed in the index.

I'm not entirely opposing the idea. It's just low-priority to me. We can introduce a status: field (k8s convention) in the Plugin object if you really want to save some dates.

I don't agree that this appears magic to users. It should be quite obvious that info does not print the install date for a plugin which is not installed. On the other hand, it makes perfect sense to show that for an installed plugin. However, it may be a good idea to show all possible output fields and leave them blank if they don't apply.

To me krew info showing Status: INSTALLED (vX.Y.Z) vs Status: NOT INSTALLED is good enough and P1, anything else is secondary. Would you agree?

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Yeah, let's keep it simple. If we really must add install metadata, we can add fields to the plugin manifest which should not be populated by plugin authors but only by krew. I don't think that install dates are important enough to mandate for a manifest change, so let's drop that.

@corneliusweig corneliusweig force-pushed the corneliusweig:install-receipt branch from 89486b3 to eb428ac Jun 21, 2019

corneliusweig added some commits Jun 18, 2019

Swap plugin install and saving receipts
Saving receipts is less prone to failure than the whole plugin
installation. When swapped, krew will have an inconsistent state in
fewer occasions.

@corneliusweig corneliusweig force-pushed the corneliusweig:install-receipt branch from 055aa1c to 6d78644 Jun 26, 2019

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2019

I've just tested this, it's mostly good to go.

I have several questions, I'm curious when do you want to address them, as they're breaking changes:

  1. Currently krew list doesn't rely on receipts to see what's installed, it scans $KREW_ROOT/bin directory.

  2. Currently krew uninstall also scans $KREW_ROOT/bin directory to see if a plugin is installed. Should it rather look for a receipt?

  3. Currently krew upgrade also scans $KREW_ROOT/bin directory which plugins are installed. It similarly infers the installed version of the plugin by looking at the symlink path of the executable instead of the receipt.

Should we do this breaking change later?

Seems like this PR does what it says ("save install receipt") as it saves the receipt on install and upgrades.

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

@ahmetb Thanks for another close look at this PR.

Storing the install receipt for upgrades should be part of this PR.

The other points you raised are all valid. But they are pretty independent, so I would rather not include them here. As stated in the first PR comment, this requires some migration of the krew folder, because we need to store the receipts for already installed plugins. This problem needs to be solved first before we can migrate the logic for list, uninstall, and upgrade.

EDIT: I opened #232 to discuss the migration plan.

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

/lgtm
/approve
Thanks for staying on top of this! 🍸

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 2, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Jul 2, 2019

[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:
  • OWNERS [ahmetb,corneliusweig]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 1831b1b into kubernetes-sigs:master Jul 2, 2019

2 of 3 checks passed

tide Not mergeable.
Details
cla/linuxfoundation corneliusweig authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ahmetb added a commit to ahmetb/krew that referenced this pull request Jul 2, 2019

pkg/installation: reorder receipt save & install()
Looks like we didn't address the code review comment at
kubernetes-sigs#195 (comment).

Copying the conversation from there:

> This is better because:
>
> - actually installing a plugin (extract files, symlink binary) but then failing
>   to store the receipt is okay (at most you'd be leaking some files)
> - claiming the plugin is installed (i.e. receipt exists) while it's not
>   installed is not okay.
> - tens of things can go wrong in `install()`, much fewer things can go wrong
>  in `receipt.Store()`, so makes sense to do it last.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>

@corneliusweig corneliusweig deleted the corneliusweig:install-receipt branch Jul 2, 2019

k8s-ci-robot added a commit that referenced this pull request Jul 2, 2019

pkg/installation: reorder receipt save & install() (#237)
* pkg/installation: reorder receipt save & install()

Looks like we didn't address the code review comment at
#195 (comment).

Copying the conversation from there:

> This is better because:
>
> - actually installing a plugin (extract files, symlink binary) but then failing
>   to store the receipt is okay (at most you'd be leaking some files)
> - claiming the plugin is installed (i.e. receipt exists) while it's not
>   installed is not okay.
> - tens of things can go wrong in `install()`, much fewer things can go wrong
>  in `receipt.Store()`, so makes sense to do it last.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>

* pkg/installation: reorder receipt saving in uninstall()

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.