mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Klues <klue...@gmail.com>
Subject Re: Review Request 57951: Moved new CLI settings into a user-defined TOML file.
Date Mon, 27 Mar 2017 23:03:42 GMT

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




src/cli_new/README.md
Lines 46-68 (patched)
<https://reviews.apache.org/r/57951/#comment243025>

    As I mentioned in the original review, I don't think we want to add these in the config
yet because none of the current plugins use them.
    
    Only once we actually introduce plugins that require these configs should we add them
here.
    
    For now, `plugins` should be the only extendable config in this file.



src/cli_new/bin/settings.py
Lines 18-20 (original), 18-20 (patched)
<https://reviews.apache.org/r/57951/#comment243026>

    This change should be part of the previous patch.



src/cli_new/bin/settings.py
Lines 46 (patched)
<https://reviews.apache.org/r/57951/#comment243027>

    s/The config file of this project/The default config file for this project./



src/cli_new/bin/settings.py
Lines 54 (patched)
<https://reviews.apache.org/r/57951/#comment243029>

    Can we reword this to:
    ```
    MESOS_CLI_CONFIG path not found: {}
    ```



src/cli_new/bin/settings.py
Line 51 (original), 62 (patched)
<https://reviews.apache.org/r/57951/#comment243028>

    This is incorrect. It should be `MESOS_CLI_CONFIG_PATH` not `os.environ.get("MESOS_CLI_CONFIG")`.



src/cli_new/bin/settings.py
Lines 71 (patched)
<https://reviews.apache.org/r/57951/#comment243030>

    We should also check, one by one, that the files pointed to by each entry in `plugins`
are directories that exist on the file system.



src/cli_new/bin/settings.py
Line 62 (original), 74 (patched)
<https://reviews.apache.org/r/57951/#comment243031>

    We should be more descriptive in the error message here:
    
    ```
    "Unable to parse config file '{}': 'plugins' field must be a list".format(MESOS_CLI_CONFIG_PATH)
    ```


- Kevin Klues


On March 27, 2017, noon, Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57951/
> -----------------------------------------------------------
> 
> (Updated March 27, 2017, noon)
> 
> 
> 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 in 'bin/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/1/
> 
> 
> Testing
> -------
> 
> Tested manually, PEP8 and Pylint used to make sure that the code style is correct.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message