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 57896: Configuration of cli_new in config.toml file.
Date Fri, 24 Mar 2017 04:17:47 GMT

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



Thanks for the review!

I think this review should actualy be broken up into three different reviews though.  We typically
like to keep each patch doing exactly one thing and I see three logical things going on in
this patch.

1) Rename config.py to settings.py in the new CLI.
2) Move new CLI settings out of settings.py into a user-defined TOML file.
3) Introduce a 'config' plugin to the new CLI.

This way you can add some descriptions about each of these logical changes in the descriptions
of the commit messages.

In the meantime, I've left some other high level comments on the current review. I'll dig
into the rest of it once it is broken up properly.


src/cli_new/README.md
Lines 46-67 (patched)
<https://reviews.apache.org/r/57896/#comment242690>

    I don't think we should introduce any of that in this commit. These should only be added
once as part of commits that actually use this functionality. In master right now, these configs
are currently meaningless and shouldn't appear here yet.
    
    The only thing that should appear here currently is `plugins`.



src/cli_new/README.md
Lines 87 (patched)
<https://reviews.apache.org/r/57896/#comment242691>

    Why `MESOS_CLI_CONFIG` instead of `MESOS_CLI_CONFIG_DIR`? Is the idea that we could keep
multiple configs in this directory and swap them out based on the environment variable?


- Kevin Klues


On March 23, 2017, 11:05 p.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57896/
> -----------------------------------------------------------
> 
> (Updated March 23, 2017, 11:05 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7269
>     https://issues.apache.org/jira/browse/MESOS-7269
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Documentation added to create a correct configuration file.
> Plugin to handle this configuration file added.
> config.py moved to settings.py to differentiate it from the plugin.
> 
> 
> Diffs
> -----
> 
>   src/cli_new/README.md 0e60515b71192ce1a544711948a5c17a6f9002af 
>   src/cli_new/bin/config.py 274f8c63b0c642637f17aa2e3c8c4a8a5a059e37 
>   src/cli_new/bin/main.py bbfb52c894540158c70e0f50ebb8a277b692d54d 
>   src/cli_new/bin/settings.py PRE-CREATION 
>   src/cli_new/lib/mesos/plugins/config/__init__.py PRE-CREATION 
>   src/cli_new/lib/mesos/plugins/config/main.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 9cc8d096ac353f4ed2d40000318f2d0e439a6edf 
> 
> 
> Diff: https://reviews.apache.org/r/57896/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