Re: Review Request 57951: Moved new CLI settings into a user-defined TOML file.

[Joseph Wu <jo...@mesosphere.io>]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57951/#review171249
-----------------------------------------------------------




src/cli_new/README.md
Lines 40 (patched)
<https://reviews.apache.org/r/57951/#comment244148>

    s/install/create/
    
    Also see my comment (much further down) about the optional/required config file.



src/cli_new/README.md
Lines 47-48 (patched)
<https://reviews.apache.org/r/57951/#comment244174>

    Looks like two tabs snuck into this file.



src/cli_new/README.md
Lines 52-53 (patched)
<https://reviews.apache.org/r/57951/#comment244149>

    TOML supports comments, so we should move this into the code block.  Doing this will eventually look neater as we add more config options (and thereby extend this template).



src/cli_new/bin/settings.py
Lines 44-46 (original), 50-57 (patched)
<https://reviews.apache.org/r/57951/#comment244173>

    Here you've changed the config file environment variable from `MESOS_CLI_CONFIG_PATH` to `MESOS_CLI_CONFIG`.
    
    Not a problem, but you'll want to call this out in the commit description.



src/cli_new/bin/settings.py
Lines 53-56 (patched)
<https://reviews.apache.org/r/57951/#comment244176>

    The config file was previously optional.  This line makes the config file *required*.
    
    The config file should be optional until there's a real need to have the user create one (before they can use the CLI).



src/cli_new/bin/settings.py
Lines 67-71 (original), 78-81 (patched)
<https://reviews.apache.org/r/57951/#comment244181>

    While it make sense to remove this environment variable (because having two ways to override a config may lead to more confusion), you should call the change out in the commit description.



src/cli_new/pip-requirements.txt
Lines 12 (patched)
<https://reviews.apache.org/r/57951/#comment244179>

    Since we're changing the config file format, you might want to include a blob in the commit description saying _why_ we want to do this in the first place.
    
    For example: 
    ```
    JSON is not particularly well-suited for holding
    configuration because:
      * JSON lacks support for comments.
      * All fields end up in a single map, with no logical grouping.
      * etc...
    ```


- Joseph Wu


On March 27, 2017, 5 a.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57951/
> -----------------------------------------------------------
> 
> (Updated March 27, 2017, 5 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7269
>     https://issues.apache.org/jira/browse/MESOS-7269
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> These settings were previously directly in `settings.py`.
> 
> 
> Diffs
> -----
> 
>   src/cli_new/README.md 0e60515b71192ce1a544711948a5c17a6f9002af 
>   src/cli_new/bin/settings.py 274f8c63b0c642637f17aa2e3c8c4a8a5a059e37 
>   src/cli_new/pip-requirements.txt 9cc8d096ac353f4ed2d40000318f2d0e439a6edf 
> 
> 
> Diff: https://reviews.apache.org/r/57951/diff/2/
> 
> 
> Testing
> -------
> 
> Tested manually, PEP8 and Pylint used to make sure that the code style is correct.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>