-
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
Populate receipt status on install/upgrade #555
Populate receipt status on install/upgrade #555
Conversation
cmd/krew/cmd/install.go
Outdated
} | ||
|
||
if *manifest != "" { | ||
plugin, err := indexscanner.ReadPluginFromFile(*manifest) | ||
if err != nil { | ||
return errors.Wrap(err, "failed to load plugin manifest from file") | ||
} | ||
install = append(install, plugin) | ||
install = append(install, receipt.New(plugin, constants.DefaultIndexName)) |
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 think we should not convert to Receipt
until we actually installed, and we're about to write the Receipt
object to disk. Thoughts @corneliusweig?
This complicates the code a bit, because nothing's installed yet, but scan index dir + convert everything to Receipt in-memory already. Looks odd in the code to me.
That's why I was imagining a small internal type, specific to this pkg that contains Plugin+indexname only.
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 something like this in mind:
type pluginEntry {
p plugin
indexName string
}
it's low effort, and doesn't overload Receipt type, just because it has a field to store the indexName. :)
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.
Ya I was going back and forth on that a lot when I was working on this. It felt weird to add a new type that was essentially the same as Receipt
but I agree that it is weird/conceptually wrong to create the receipt before the plugin is actually installed.
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.
Yeah, this is definitely cleaner. Just because Receipt
happens to have the field that we want to store in addition does not mean it is the right choice. Consider what happens when we add more fields to Receipt
. What would happen most likely is that we would not update this instance and then we have a in invalid Receipt
in our hands. So a dedicated struct is what we need here.
internal/installation/install.go
Outdated
@@ -54,7 +54,8 @@ var ( | |||
|
|||
// Install will download and install a plugin. The operation tries | |||
// to not get the plugin dir in a bad state if it fails during the process. | |||
func Install(p environment.Paths, plugin index.Plugin, opts InstallOpts) error { | |||
func Install(p environment.Paths, pluginReceipt index.Receipt, opts InstallOpts) error { |
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.
this should not receive a Receipt, conceptually sounds wrong.
"sigs.k8s.io/krew/pkg/index" | ||
) | ||
|
||
// Store saves the given plugin receipt at the destination. | ||
// The caller has to ensure that the destination directory exists. | ||
func Store(plugin index.Plugin, dest string) error { | ||
func Store(plugin index.Receipt, dest string) error { |
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.
you might wanna rename variable name to receipt
as well + update godoc. :)
if name == "" { | ||
name = constants.DefaultIndexName | ||
} |
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.
under what condition we specify "" ?
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.
You're right, I forgot the CanonicalPluginName
function returns the default index already. I can remove this.
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.
Even without that, this should not happen. We'll always have an index name in pkg/installation.
CanonicalPluginName
should be used only for turning user input (e.g. install args) to full plugin names.
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.
It sounds like you are talking about the same thing. Anyway, everybody agrees that the check name == ""
is unnecessary, because it is covered elsewhere.
install = append(install, plugin) | ||
install = append(install, pluginEntry{ | ||
p: plugin, | ||
indexName: constants.DefaultIndexName, |
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.
this is a case we did not think of.
I guess we can keep shoving them into default. thoughts @corneliusweig ?
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.
This is what happens at the moment. But we can use the opportunity to separate this better from the default
index. So instead, what about setting the indexName to detached
or something similar?
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.
++ @chriskim06 do you mind making a small PR?
Right now we lack any integration tests for the changes. Note that you're changing criticial path in this PR. So we need to make sure newly added behavior works. We can merge, but the very next PR we need would be the integration tests. Some basic cases I'm thinking easily:
|
I can add the integration tests here or in a separate one if that'll make it easier. Let me know which one you'd prefer. |
cmd/krew/cmd/install.go
Outdated
for _, plugin := range install { | ||
klog.V(2).Infof("Will install plugin: %s\n", plugin.Name) | ||
for _, pluginEntry := range install { | ||
klog.V(2).Infof("Will install plugin: %s\n", pluginEntry.p.Name) |
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.
this will omit the index name.
I'm curious how you tested this currently. I'm trying to get it to work locally and tried a fake local index at a directory, and wasn't able to install. So the integration tests can actually verify it works. |
That's strange. I pretty much did what you mentioned and what's in the integration test: add index then install and it worked. |
/lgtm nice and small patch! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, chriskim06 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 |
While testing locally I noticed that if I create a fake index dir, do So we need to document that while testing with local index repos, they need to commit the changes, so that the |
Related issue: #483