Add support for write only configuration profiles. #64
Conversation
andrewmchen
force-pushed
the
configure-hosts
branch
2 times, most recently
from
d5733a6
to
8da6681
Compare
databricks_cli/configure/cli.py
Outdated
| @@ -35,41 +35,42 @@ | |||
| PROMPT_TOKEN = 'Token' # NOQA | |||
|
|
|||
|
|
|||
| def _configure_cli_token(): | |||
| def _configure_cli_token(profile): | |||
There was a problem hiding this comment.
does it support consolidated shard? I think we need to set workspaceId for it.
There was a problem hiding this comment.
Yeah it should. The profile here is just to distinguish between different types of configurations. Since the DEFAULT profile already works with consolidated I don't see why other profiles wouldn't.
databricks_cli/configure/config.py
Outdated
| @@ -51,30 +52,35 @@ def decorator(*args, **kwargs): | |||
| return decorator | |||
|
|
|||
|
|
|||
| def _get_api_client(): | |||
| def profile_option(f): | |||
There was a problem hiding this comment.
Why don't we just document how to create config file? I don't think the command option is that useful for this. As a user I would be fine to copy paste and modify the token.
There was a problem hiding this comment.
I think that would be useful as well. Regarding the command option, aws configure supports the --profile option so this PR follows this convention.
tests/configure/test_cli.py
Outdated
| TEST_USER + '\n' + | ||
| TEST_PASSWORD + '\n' + | ||
| TEST_PASSWORD + '\n')) | ||
| assert DatabricksConfig.fetch_from_fs().host(DEFAULT_SECTION) == TEST_HOST |
There was a problem hiding this comment.
can we have some test that doesn't use DEFAULT_SECTION
There was a problem hiding this comment.
test_configure_two_sections should test that behavior.
tests/configure/test_config.py
Outdated
|
|
||
| def test_is_valid_true(self): | ||
| assert self.databricks_config_from_password.is_valid | ||
| assert self.databricks_config_from_token.is_valid | ||
|
|
||
| def test_is_valid_false(self): | ||
| databricks_config = config.DatabricksConfig() | ||
| assert not databricks_config.is_valid | ||
| assert not databricks_config.is_valid(config.DEFAULT_SECTION) |
There was a problem hiding this comment.
Do we need DEFAULT_SECTION everywhere? Can we simply call assert not databricks_config.is_valid()
There was a problem hiding this comment.
IMO, the explicit parameter adds some clarity here.
andrewmchen
force-pushed
the
configure-hosts
branch
from
8da6681
to
2a44e78
Compare
There was a problem hiding this comment.
'DEFAULT' is the default profile name. But we should think about whether it should be case sensitive or not. Currently, if I ran databricks configure --profile default, I got
File "/Users/meng/anaconda2/lib/python2.7/site-packages/databricks_cli-0.4.2-py2.7.egg/databricks_cli/configure/config.py", line 131, in _create_section_if_absent
self._config.add_section(profile)
File "/Users/meng/anaconda2/lib/python2.7/ConfigParser.py", line 261, in add_section
raise ValueError, 'Invalid section name: %s' % section
ValueError: Invalid section name: defaul
databricks_cli/configure/config.py
Outdated
| @property | ||
| def host(self): | ||
| return self._config.get(DEFAULT_SECTION, HOST) if self.is_valid else None | ||
| def host(self, profile): |
There was a problem hiding this comment.
Seems more extensible if we implement a DatabricksConfigProvider class (or Factory) that has def from_profile(profile): method. Then the code changes to
conf = DatabricksConfProvider.from_profile(profile)
api_client = ApiClient(conf)
andrewmchen
force-pushed
the
configure-hosts
branch
from
4cd6553
to
f467102
Compare
This is part one of two for the configuration profile feature.
In this part, we add support for writing configurations with different profiles. In the next PR, we will add support for using this profile to authenticate to a Databricks workspace other than the one specified by the
DEFAULTprofile.