Add prompting mechanism for config values and add global options: --config, --server, --username, --password, --debug #98
Conversation
| elsif v == '${PROMPT_PASSWORD}' | ||
| @config[k.to_sym] = password("Enter a value for #{k}:") | ||
| else | ||
| @config[k.to_sym] = v if @valid_config_keys.include? k.to_sym |
There was a problem hiding this comment.
Convert if nested inside else to elsif.
msabramo
force-pushed
the
password_ask
branch
2 times, most recently
from
747dc8a
to
e3cfb1a
Compare
| def test_that_config_loads_from_yaml | ||
| assert_equal ({ server: '127.0.0.1', port: 8084 }), | ||
| @test_aptly_load.configure_with('test/fixtures/aptly-cli.yaml') | ||
| end | ||
|
|
||
| def test_that_config_loads_from_yaml_with_prompts | ||
| assert_equal ({:server=>"127.0.0.1", :port=>8084, | ||
| :username=>"zane", :password=>"${PROMPT}"}), |
There was a problem hiding this comment.
Use the new Ruby 1.9 hash syntax.
Surrounding space missing for operator =>.
Space inside } missing.
Prefer single-quoted strings when you don't need string interpolation or special symbols.
| location.to_s, | ||
| 'test_1.0_amd64.deb', | ||
| 'test/fixtures/test_1.0_amd64.deb') | ||
| AptlyCli::AptlyFile.new |
There was a problem hiding this comment.
Inconsistent indentation detected.
This allows not keeping passwords and such in the config file.
E.g.: In `/etc/aptly-cli.conf`:
```yaml
---
:proto: http
:server: 10.3.0.46
:port: 8082
:debug: false
:username: ${PROMPT}
:password: ${PROMPT_PASSWORD}
```
The tool will prompt for the specified values -- e.g.:
```
$ aptly-cli version
Enter a value for username:
zane
Enter a value for password:
***************
{"Version"=>"0.9.7"}
```
This also eliminates a bunch of duplicated code in all of the command
classes by having them inherit from a common base class. This was
actually essential because otherwise, every required file would trigger
a config load and thus prompting for any needed values over and over
again.
| @@ -36,7 +43,8 @@ def configure_with(path_to_yaml_file) | |||
| config = YAML.load(IO.read(path_to_yaml_file)) | |||
| rescue Errno::ENOENT | |||
| @log.warn( | |||
| 'YAML configuration file couldn\'t be found at /etc/aptly-cli.conf. Using defaults.') | |||
| "YAML configuration file couldn\'t be found at " + | |||
There was a problem hiding this comment.
Use \ instead of + or << to concatenate those strings.
This lets you specify a different config file. E.g.:
aptly-cli -c ~/.config/aptly-cli/aptly-cli.conf repo_list
msabramo
changed the title
| @@ -0,0 +1,40 @@ | |||
| class AptlyCommand | |||
| def initialize(config, options=nil) | |||
There was a problem hiding this comment.
Surrounding space missing in default value assignment.
This required a fairly big refactor, because I had to move stuff that
processes `${PROMPT}` and `${PROMPT_PASSWORD}` from `aptly_load.rb` to
`aptly_command.rb`, so that it could be used in the global options as
well as in the configuration file.
msabramo
changed the title
|
This is great! Looks good first pass on my phone. I'll take a closer look this weekend or Monday for sure. |
| keyring = Keyring.new | ||
| value = keyring.get_password(@config[:server], @config[:username]) | ||
|
|
||
| if !value |
There was a problem hiding this comment.
Favor unless over if for negative conditions.
Optionally use the [`keyring` gem](https://github.com/jheiss/keyring) when `${KEYRING}` is specified as a value for an option or config setting. Note that I require `keyring` at the point of use, so that it is an optional dependency.
|
Ok, this looks good to merge to me! Should we add your pull request notes to the README.md for docs too? I can merge this and #101 then release gem 0.3.0. Ill tag the release with notes on all of the changed add. Thanks again, this is a great security and functionality addition! |
|
#102 for adding to README.md this feature |
|
#105 documents the keyring stuff in README.md |
This allows not keeping passwords and such in the config file.
E.g.: In
/etc/aptly-cli.conf:The tool will prompt for the specified values, where
${PROMPT}results in a regular prompt and${PROMPT_PASSWORD}results in a password prompt where the input is replaced by asterisks -- e.g.:This also eliminates a bunch of duplicated code in all of the command classes by having them inherit from a common base class. This was actually essential because otherwise, every required file would trigger a config load and thus prompting for any needed values over and over again.
The
--config(-c) option allows specifying an alternative config file -- e.g.:and the
--server,--username, and--passwordoptions allow specifying those things on the command-line and not even requiring a config file:Note that you can use
${PROMPT}and${PROMPT_PASSWORD}in the values of these options, just as you can in a config file.