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

lib/config: Don't migrate non-HTTPS-URL discovery servers to new path #3104

Closed
wants to merge 2 commits into from
Closed

Conversation

calmh
Copy link
Member

@calmh calmh commented May 17, 2016

Purpose

Don't screw up unknown URLs in the migration (i.e. default-v4 -> default-v4v2/). Don't panic.

Testing

Tested manually on a v13 config...

panic(err)
}
uri.Path += "v2/"
uri, err := url.Parse(addr)
Copy link
Member Author

Choose a reason for hiding this comment

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

"default" and so on are valid URLs with an empty scheme and will slip through this untouched.

@AudriusButkevicius
Copy link
Member

I guess I'd rather bump config version and release 13.1?

@calmh
Copy link
Member Author

calmh commented May 17, 2016

Why increment config? To fix it back you mean? I don't think we have that many users with default-v4 or default-v6 in their configs...

@calmh
Copy link
Member Author

calmh commented May 17, 2016

Well, I guess it doesn't hurt to do so anyway just to capture those two cases.

@AudriusButkevicius
Copy link
Member

Ok, but still lets do a 0.13.1 so this wouldn't be contagious?

@st-review merge

@calmh
Copy link
Member Author

calmh commented May 17, 2016

I'd rather wait a little longer to see what else pops up so we don't have to release a .2 in fifteen minutes. We can't release much more before we need to start removing releases or we'll leave v0.12.23 behind :(

@calmh
Copy link
Member Author

calmh commented May 17, 2016

Hang on and I'll add a v15 migration

@calmh calmh closed this May 17, 2016
@calmh calmh reopened this May 17, 2016
@AudriusButkevicius
Copy link
Member

I guess we can remove 0.13.0 once 0.13.1 is out?

@calmh
Copy link
Member Author

calmh commented May 17, 2016

There. Fixed in the original, and an additional to undo the damage.

@calmh
Copy link
Member Author

calmh commented May 17, 2016

Yeah, we could. It's a bit ugly though. I think we can wait a day and collect a few more fixes into .1.

@AudriusButkevicius
Copy link
Member

@st-review merge

@st-review
Copy link

👌 Merged as 922e140. Thanks, @calmh!

st-review pushed a commit that referenced this pull request May 17, 2016
@st-review st-review closed this May 17, 2016
@calmh calmh deleted the fix3103 branch May 21, 2016 01:28
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Jun 16, 2017
@syncthing syncthing locked and limited conversation to collaborators Jun 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants