-
Notifications
You must be signed in to change notification settings - Fork 66
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
refactor: require migration, remove path factory #1389
Conversation
b471d00
to
647d019
Compare
This commit puts migration code to use, marking the migration as required before qri will start. This migration does a few things: * runs ipfs repo migration to support go-ipfs v0.5.0 * converts ipfs configuration to use non-standard IPFS ports * moves the IPFS repo into the qri folder * adds a "Filesystems" configuration field Qri now only honors "QRI_PATH" by default. Drop cmd.EnvPathFactory in favour of qri path functions & string values.
647d019
to
7863cd6
Compare
msg := `Your repo needs updating before qri can start. | ||
Run migration now?` | ||
if !confirm(streams.Out, streams.In, msg) { | ||
return qerr.New(ErrNeedMigration, `your repo requires migration before it can run`) |
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.
Something clearer.
'Your repo needs to be migrated before any qri commands can be run.'
Or, from line 25:
'Your repo needs updating before qri can start.'
They should probably say the same thing.
Or maybe ErrNeedMigration
should be more explainy since they answered ‘no’ to the simple explanation. Maybe also a linefeed before.
What it looks like currently:
❯ qri list
Your repo needs updating before qri can start.
Run migration now? [Y/n]: n
your repo requires migration before it can run
Notes here. The error for network failure is a little ugly. Probably not worth doing anything about it right now. @ me if you want me to file an issue. |
I've got this:
Also got:
On a fresh pre-migration qri setup (ie just did |
here's what I suspect happened @Arqu: my guess: you had a config file with a
This is a different (more concerning) issue, I'm going to try to reproduce now. |
Ok, first effort to reproduce looks like it's working ok. Here's (roughly) what I ran:
|
Yup, changing the config revision to 1 and re-doing the migration did the trick. |
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.
There's a big move away from the term "repoPath" here. Is that motivated by deemphasizing the repo
package? Because I think referring to the place that stores block data, logbook, refs, config, etc as the "repository" is still useful as a piece of terminology. In fact, it's even more useful once "ipfs/" lives inside "qri/", since then there's no confusion over whether "repo" refers to "qri/" or "ipfs/" or the parent of one or both of these.
While QRI_PATH as an environment variable makes sense - it matches of a standard convention of specifying configuration and data - out of that context, seeing "qriPath" as a method name is clear obvious. It may not be too confusing within this isolated change, but for example, in Desktop it could mean "path to the qri binary", which would become tricky when working on the "qri cli" and "Desktop" at the same time.
cmd/qri.go
Outdated
@@ -36,8 +34,7 @@ https://github.com/qri-io/qri/issues`, | |||
cmd.SetUsageTemplate(rootUsageTemplate) | |||
cmd.PersistentFlags().BoolVarP(&opt.NoPrompt, "no-prompt", "", false, "disable all interactive prompts") | |||
cmd.PersistentFlags().BoolVarP(&opt.NoColor, "no-color", "", false, "disable colorized output") | |||
cmd.PersistentFlags().StringVar(&opt.RepoPath, "repo", qriPath, "provide a path to load qri from") | |||
cmd.PersistentFlags().StringVar(&opt.IpfsPath, "ipfs-path", ipfsPath, "override IPFS path location") | |||
cmd.PersistentFlags().StringVar(&opt.qriPath, "path", qriPath, "provide a path to load qri from") |
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 description and flag name seems too generic and/or confusing. This sounds like it could be used to load an executable or framework or something. And path
doesn't narrow down what is happening. What if we use qri-path
and describe it as "path where qri data and config lives"
cmd/qri.go
Outdated
@@ -162,6 +155,11 @@ func (o *QriOptions) Instance() *lib.Instance { | |||
return o.inst | |||
} | |||
|
|||
// QriPath returns the path to the qri directory |
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.
Would prefer some more specificity. This could mean the "qri directory" where the binary is installed, or where Desktop exists, or where I store my datasets. How about "QriPath returns the path where qri data and configuration exist"
@@ -291,7 +287,7 @@ func NewInstance(ctx context.Context, repoPath string, opts ...Option) (qri *Ins | |||
opts = []Option{ | |||
OptStdIOStreams(), | |||
OptSetIPFSPath(""), | |||
OptCheckConfigMigrations(), | |||
OptCheckConfigMigrations(false), |
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.
What are your thoughts on changing the migration UI such that after migrations run successfully the qri binary exits with a message like "migration successful, please run command again". As it is now, if I run qri list
then I get the migration prompt, confirm, see some messages about progress, then soon after the pager pops up and obscures what happened. I think it would be fine to bail out inside and ask the user to start over, it's less cognitive load than having to remember what was going on before the migration started.
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.
+1
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.
"migration successful, please run command again"
fully agree. Ok if I do this as a follow-up 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.
nm, managed to fit this in
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.
Yes totally
@@ -51,8 +51,14 @@ func newTempRepo(peername, prefix string, g gen.CryptoGenerator) (r TempRepo, er | |||
return r, err | |||
} | |||
|
|||
// Create directory for new Qri repo. | |||
QriPath := filepath.Join(RootPath, "qri") |
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.
lower-case this var
Hmmm, why is RootPath
upper-case?
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.
uhhh... dunno. will fix
cmd/factory.go
Outdated
return filepath.Join(dir, "qri"), filepath.Join(dir, "ipfs") | ||
} | ||
// RootedQriPath gives a "/qri" directory from a given root path | ||
func RootedQriPath(root string) string { |
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 only used in one place, might as well remove it and inline at the call site.
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.
Besides the existing comments mostly LGTM. I just have a few more to make sure we don't slip.
config/config.go
Outdated
{ | ||
Type: "ipfs", | ||
Config: map[string]interface{}{ | ||
"path": "./ipfs", |
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.
Is this path "correct"? In the sense that I'm not sure what it's relative to and also not sure whether this works well on windows.
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.
really good point about windows. At a minimum we should audit to make sure this value is always generated with file path.Join(cfg.Path(), "ipfs")
.
Regarding "correct", this is what we said in RFC0029:
Relative Paths in configuration files are relative to the file
If a configuration value specifies a filepath, and the given path is relative, that path should be relative to the location of the config file. We should add a note of this to $ qri config --help.
@@ -87,7 +108,6 @@ func OneToTwo(cfg *config.Config) error { | |||
} | |||
|
|||
// TODO(ramfox): remove original ipfs repo after all migrations were successful |
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.
Is this handled now? If yes we should remove the comment, if not, think this definitely needs to get in.
Also thinking about it now, what happens if users use ipfs for other stuff too? Should we or should we not remove it?
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's not yet, and I agree we need to figure out an answer here. whatever we come up with, I'd like to handle it in a separate 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.
Works for me, it's safe for the end user even in this state, just that we waste some disk space currently.
@@ -291,7 +287,7 @@ func NewInstance(ctx context.Context, repoPath string, opts ...Option) (qri *Ins | |||
opts = []Option{ | |||
OptStdIOStreams(), | |||
OptSetIPFSPath(""), | |||
OptCheckConfigMigrations(), | |||
OptCheckConfigMigrations(false), |
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.
+1
Definitely motivated by de-emphasizing the term "repo". I was hoping to make the change in terminology incrementally. After reading your note I agree this PR doesn't even address that change in the notes. which isn't good. If we're going to drop the "repo" term, we should do it explicitly. I'll revert changes away from the |
63dcf97
to
a4b8868
Compare
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.
LGTM, one comment about the "return successfully after migration".
@@ -60,6 +61,9 @@ func Execute() { | |||
// ErrExit writes an error to the given io.Writer & exits | |||
func ErrExit(w io.Writer, err error) { | |||
log.Debug(err.Error()) | |||
if errors.Is(err, migrate.ErrMigrationSucceeded) { | |||
printSuccess(w, "migration succeeded, re-run your command to continue") |
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.
Add os.Exit(0)
here? Otherwise looks like the err gets printed again. Also, return code should be 0 on success. Though I'm not sure if "the original command didn't run" should count as success?
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.
damn nice catch. I vote we exit with status 0 (success), a thing did succeed, and only because the user chose Yes
. I've intentionally carved around a non interactive version of this so we can write qri connect --migrate
which will auto-migrate and continue
this way users don't get super confused. restore "RepoPath" to describe qri data directory
a4b8868
to
4aafc9c
Compare
Put migration code to use, marking the migration as required before qri will start. This migration does a few things:
Qri now only honors
QRI_PATH
by default. Drop cmd.EnvPathFactory in favour of qri path functions & string values.closes #1296, closes #1319, closes #1346