osx: stop the redirector before an upgrade #146
Conversation
Issue: KBFS-2787
Issue: KBFS-2787
| @@ -183,6 +183,12 @@ func (c context) stop() error { | |||
| c.log.Warningf("Error requesting app exit: %s", appExitErr) | |||
| } | |||
|
|
|||
| // Stop the redirector so it can be upgraded for all users. | |||
There was a problem hiding this comment.
It looks like you're uninstalling the redirector first, not just stopping it. That's fine, but the comments and error messages should reflect what's actually happening.
Also, what happens if the upgrade fails?
There was a problem hiding this comment.
One more question: will this work before a redirector exists?
There was a problem hiding this comment.
It looks like you're uninstalling the redirector first, not just stopping it. That's fine, but the comments and error messages should reflect what's actually happening.
Under the covers, --uninstall-redirector just calls stopRedirector in the root helper. It's following the naming convention of everything else, including --uninstall-kbfs which just stops KBFS but doesn't actually remove anything from the system.
Also, what happens if the upgrade fails?
Then the redirector will remain shut down until the next time the app starts up. I think this matches the behavior of shutting down the rest of the application before the upgrade happens.
One more question: will this work before a redirector exists?
Nope. That's why I mention in the PR description that I won't merge this until the redirector changes are merged into master.
| @@ -183,6 +183,12 @@ func (c context) stop() error { | |||
| c.log.Warningf("Error requesting app exit: %s", appExitErr) | |||
| } | |||
|
|
|||
| // Stop the redirector so it can be upgraded for all users. | |||
| _, redirectorErr := command.Exec(c.config.keybasePath(), []string{"uninstall", "--components=redirector"}, time.Minute, c.log) | |||
There was a problem hiding this comment.
Is c.config.keybasePath() the keybase binary? Out of curiosity I did keybase uninstall and it seemed to be removing a bunch of binaries from my system as part of uninstalling the app. So it sounds like keybase uninstall is different from "uninstalling" a mount?
There was a problem hiding this comment.
Is c.config.keybasePath() the keybase binary?
Yep.
So it sounds like keybase uninstall is different from "uninstalling" a mount?
keybase uninstall without any flags attempts to do a full uninstall. But if you give it --components, you can do smaller things, like stopping KBFS, or uninstalling just the fuse module, or deleting just the mount directory. So I added a component for stopping the redirector.
There was a problem hiding this comment.
Got you. Thanks for explaining it. So there are two levels of "uninstall":
-
Stop the redirector and removing the dir that corresponds to the mount point.
-
Remove the keybase-redirector itself.
It seems uninstalling the redirector component is 1. I suppose we still need 2 somewhere?
There was a problem hiding this comment.
It seems uninstalling the redirector component is 1. I suppose we still need 2 somewhere?
Maybe, though it would be as part of one of the other PRs in client. I can test it out and see if that binary is treated differently from the others.
|
Shit, merged the wrong thing. Reverting. |
Note that this shouldn't be merged until keybase/client#10648 is merged into client master. But it's good for review now.
Issue: KBFS-2787