Remove receipts migration #603
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XL
There was a problem hiding this comment.
I'm feeling a bit uneasy about removing the migration code without having the new one in place. First, it will make it more difficult to re-use the deleted utils and tests for the upcoming automatic migration logic.
But more importantly, if we merge this, we will have to bump the minor version if we have to craft a new release. Patch-level will no longer be correct, because we are breaking something.
Lastly, I think the migration instructions are broken. Or am I wrong here?
| @@ -72,6 +54,5 @@ func init() { | |||
| if _, ok := os.LookupEnv(constants.EnableMultiIndexSwitch); ok { | |||
| systemCmd.AddCommand(indexUpgradeCmd) | |||
| } | |||
| systemCmd.AddCommand(receiptsUpgradeCmd) | |||
| rootCmd.AddCommand(systemCmd) | |||
There was a problem hiding this comment.
I'd prefer to remove the whole system command. It would make sense to do that in a follow-up PR right after this one, so that we can revert the PR lateron if we find that we need system again.
However, as long as it does nothing, there's no point in leaving it here.
There was a problem hiding this comment.
Ya I was planning on removing it in the follow up PR where I make the index migration automatic.
| @@ -16,25 +16,9 @@ | |||
| package receiptsmigration | |||
There was a problem hiding this comment.
IMHO, if we dump the migration logic, we can also dump all checks if KREW_HOME has been migrated, because we don't offer a good way forward. If somebody with an old krew tries to upgrade, they will end up with a broken installation and have to recover manually.
There was a problem hiding this comment.
I think having Done is useful to be able to detect when to guide 0.2 users on how to manually reinstall krew/plugins right? That's the reason I left this in.
cmd/krew/cmd/root.go
Outdated
| fmt.Fprintln(os.Stderr, "You need to perform a migration to continue using krew.\nPlease run `kubectl krew system receipts-upgrade`") | ||
| if !isMigrated { | ||
| fmt.Fprintln(os.Stderr, `This version of Krew is not supported anymore. Please manually migrate: | ||
| 1. Backup plugin list: kubectl krew list > backup.txt |
There was a problem hiding this comment.
So here's the problem: krew list will not work, because the krew we're running here is a new one, but it is faced with an old index. Instead of listing plugins, krew list will simply produce this error message and bail out. That's the reason why I was advocating for a unified migration.
The instructions here cannot rely on any krew command and must rely on bash (BSD flavor, so that OSX works) and PowerShell or other Windows instructions.
There was a problem hiding this comment.
Ah good catch, I can change this. I can come up with something for bash, but I don't really have any experience in windows 😅. I'll try to figure out what it would look like there, but do you or @ahmetb know what the windows equivalent of ls -d $KREW_HOME/store/ > plugins.tmp/kubectl krew install < plugins.tmp would be?
There was a problem hiding this comment.
Nice catch.
I'd say don't bother with this and just talk about:
- uninstall...
- reinstall...
- install the plugins you used
(don't mention backups).
cmd/krew/main.go
Outdated
| @@ -22,5 +22,5 @@ import ( | |||
|
|
|||
| func main() { | |||
| cmd.Execute() | |||
| defer klog.Flush() | |||
| klog.Flush() | |||
There was a problem hiding this comment.
🤔 let's move this 1 above maybe and keep as defer.
there might be some os.Exit/panics causing klog.Flush to not be called
There was a problem hiding this comment.
Sounds good. For some reason the linter started complaining about the defer being at the end of the function
$ hack/run-lint.sh
cmd/krew/main.go:25:2: unnecessaryDefer: defer klog.Flush() is placed just before return (gocritic)
defer klog.Flush()
I'll do as you suggested 👍
chriskim06
force-pushed
the
remove-receipts-migration
branch
from
68dcb1a to
f145437
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[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 |
k8s-ci-robot
added
the
approved
This removes the receiptsmigration code like we discussed in the related issue. I'm leaving the
system.gofile for now and leaving the integration test with the same name, but I might end up changing it in a follow up PR when I switch the index migration to be automatic.Related issue: #600