New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a 'central' .cs.json for cs command. #1074

Merged
merged 3 commits into from Feb 7, 2019

Conversation

Projects
None yet
4 participants
@yueri
Copy link
Contributor

yueri commented Feb 5, 2019

Changes proposed in this PR

  • Introduces a central .cs.json file for the cs command

Why are we making these changes?

We want to have a default .cs.json configuration file for all users. This allows users to roll out updates to their configurations (for instance, adding a new cluster) by only updating the default .cs.json file rather than the .cs.json file of all their end users.

@@ -17,7 +17,8 @@ Custom defaults may be provided for all commands.
Multiple clusters are supported via configuration.

In order to use the Cook CLI, you’ll need a configuration file.
`cs` looks first for a `.cs.json` file in the current directory, and then for a `.cs.json` file in your home directory.

This comment has been minimized.

@yueri

yueri Feb 5, 2019

Author Contributor

I'm not very happy with the wording here. Can someone think of a better way to phrase this?

This comment has been minimized.

@dposada

dposada Feb 5, 2019

Member

I think this wording is fine. Maybe it would help to add the "why" from your PR:

Why are we making these changes?

We want to have a default .cs.json configuration file for all users. This allows users to roll out updates to their configurations (for instance, adding a new cluster) by only updating the default .cs.json file rather than the .cs.json file of all their end users.

@dposada
Copy link
Member

dposada left a comment

Code LGTM.

Can we write integration test(s) for this?

@dposada dposada added cli wip labels Feb 5, 2019

@yueri

This comment has been minimized.

Copy link
Contributor Author

yueri commented Feb 6, 2019

I'm still working on the integration tests. It took me a while to figure out how they were working in the first place, but I think I have an idea for adding some checks.

@yueri yueri force-pushed the yueri:master branch from 6a393cf to 97f1833 Feb 7, 2019

@pschorf

pschorf approved these changes Feb 7, 2019

@pschorf pschorf merged commit 8d305f9 into twosigma:master Feb 7, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment