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

fix(config migration): add config migration to update p2p.QriBootstrapAddrs #636

Merged
merged 2 commits into from
Dec 12, 2018

Conversation

b5
Copy link
Member

@b5 b5 commented Dec 12, 2018

This PR adds a Version field the config file, and migrates any configuration that doesn't have

@b5 b5 added the fix A bug fix label Dec 12, 2018
@b5 b5 requested review from dustmop and ramfox December 12, 2018 14:33
@ghost ghost assigned b5 Dec 12, 2018
@ghost ghost added the in progress label Dec 12, 2018
Copy link
Contributor

@dustmop dustmop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, have one concern then some small nits.

config/config.go Outdated
@@ -15,8 +15,12 @@ import (
"github.com/qri-io/jsonschema"
)

// CurrentConfigVersion is the latest configuration number
const CurrentConfigVersion = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this might be a minor quibble, but I could see an issue with calling this "version". These days, version tends to imply something like semver, and is expected to rarely change its major value, incrementing a minor number more often. Of course we're only using the term "version" internally, but it's still a visible term inside the textual config file. As an alternative, we could use "revision", which is more often used as a monotonically increasing value, like for example a build number.

I don't feel super strongly about this, just a thought.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh I love this. Seeing the word "Version" not followed by a semver is silly. Will Change.

lib/config.go Outdated
@@ -4,6 +4,9 @@ import (
"encoding/json"
"fmt"

"github.com/qri-io/ioes"
"github.com/qri-io/qri/config/migrate"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra blank line?

@@ -7,6 +7,8 @@ import (
"os"
"testing"

"github.com/qri-io/ioes"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra blank line?

@b5 b5 merged commit 3ea2244 into master Dec 12, 2018
@ghost ghost removed the in progress label Dec 12, 2018
@b5 b5 deleted the config_migrations branch December 12, 2018 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants