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

cmd: Add check for newer config file and an option to override it (fixes #4921) #5597

Merged
merged 4 commits into from Mar 12, 2019

Conversation

nekr0z
Copy link
Contributor

@nekr0z nekr0z commented Mar 11, 2019

Testing

Compiled and tested manually on linux-amd64, works as expected.

@AudriusButkevicius
Copy link
Member

The error message should print the versions and suggest to use the option only of this is expected.

@nekr0z
Copy link
Contributor Author

nekr0z commented Mar 11, 2019 via email

@nekr0z
Copy link
Contributor Author

nekr0z commented Mar 11, 2019

Done.

Copy link
Member

@AudriusButkevicius AudriusButkevicius left a comment

Choose a reason for hiding this comment

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

In principle looks ok, other than the nitpick.

I wonder about refusing future configs via api like explained in the ticket, but I guess that's not very often that it would happen, as most users get, modify, post back.

cmd/syncthing/main.go Outdated Show resolved Hide resolved
@nekr0z
Copy link
Contributor Author

nekr0z commented Mar 11, 2019

I wonder about refusing future configs via api like explained in the ticket, but I guess that's not very often that it would happen, as most users get, modify, post back.

Should be doable, but lib/config is a little bit beyond my speaking go just yet. :(

@AudriusButkevicius
Copy link
Member

I think -allow-newer-config would be the more correct term, as -allow-new-config is a bit ambigious, for example, if we already have a database, but no config in some distant future we might refuse to start, and option like that might be used to allow creation of a new config.

Again, I know I am annoying and it's a nit-pick, but I just want to be extra clear what the option means.

@AudriusButkevicius
Copy link
Member

AudriusButkevicius commented Mar 11, 2019

Sorry, given you are new contributor, I tend to look at PRs with more details and be annoying and picky :(

@nekr0z
Copy link
Contributor Author

nekr0z commented Mar 12, 2019

Sorry, given you are new, I tend to look at PRs with more details and be annoying and picky :(

Not only is it OK, but thank you for that! As a developer, I'm only starting to learn how to do things, and your input helps me a lot. As a user, the tighter code review the better.

@AudriusButkevicius AudriusButkevicius merged commit 04f05f1 into syncthing:master Mar 12, 2019
@nekr0z nekr0z deleted the newerconfig branch March 12, 2019 07:18
@calmh calmh added this to the v1.1.1 milestone Mar 12, 2019
@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 Mar 12, 2020
@syncthing syncthing locked and limited conversation to collaborators Mar 12, 2020
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

4 participants