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

CLI options now override env vars #718

Merged
merged 2 commits into from
Jan 2, 2017
Merged

CLI options now override env vars #718

merged 2 commits into from
Jan 2, 2017

Conversation

mholt
Copy link
Contributor

@mholt mholt commented Jan 2, 2017

In testing restic, I had an env variable RESTIC_REPOSITORY that was my "main" or "real" backup location, but then I ran restic --repo /other/repo/for/testing init and it still tried to use the value in RESTIC_REPOSITORY. This change re-prioritizes CLI flags over env vars, which in my opinion, is more intuitive.

I hope it's on the right track, I noticed that the parseEnvironment() function didn't use any of its parameters and I didn't see why it needed to happen outside of the init().

Let me know if you need anything in the way of testing this.

@codecov-io
Copy link

codecov-io commented Jan 2, 2017

Current coverage is 52.97% (diff: 50.00%)

Merging #718 into master will increase coverage by 0.19%

@@             master       #718   diff @@
==========================================
  Files            91         91          
  Lines          7242       7238     -4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3822       3834    +12   
+ Misses         2870       2850    -20   
- Partials        550        554     +4   

Powered by Codecov. Last update 04846b1...0a34a2d

@fd0
Copy link
Member

fd0 commented Jan 2, 2017

Oh, that's really embarrassing. Thanks for raising this issue. The order should obviously be command-line params before environment.

This change however breaks restic: The environment is never considered. The struct globalOptions is filled by parseEnvironment(), but the flag definitions seem to reset the values to the default values specified in the call to StringVarP(). With parseEnvironment() in the PersistentPreRunE hook, it is run after init(), but before any command code is run, so it worked before.

So, for a proper fix I suggest removing RESTIC_REPOSITORY from parseEnvironment() and changing the line defining the --repo flag as follows:

f.StringVarP(&globalOptions.PasswordFile, "password-file", "p", os.Getenv("RESTIC_REPOSITORY"), "read the repository password from a file")

We can then also fold back the one call to os.Getenv() for RESTIC_PASSWORD back into the init() function.

That should fix it. Would you like to push another commit with the change?

@mholt
Copy link
Contributor Author

mholt commented Jan 2, 2017

Oops, I thought I took care of that too, but after I re-exported the env variable I didn't hit [return] to run the restic command and check; so you're right. Now I'm embarrassed 😄

Pushed another commit, and was sure to test it this time. :)

Copy link
Member

@fd0 fd0 left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

@fd0 fd0 merged commit 0a34a2d into restic:master Jan 2, 2017
fd0 added a commit that referenced this pull request Jan 2, 2017
CLI options now override env vars
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants