----------------------------------------------------------- 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 > >
