Adding E2/MWS Accounts API support to the CLI #294
Conversation
databricks_cli/accounts/__init__.py
Outdated
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| from .api import * |
There was a problem hiding this comment.
Hm, why do we need this?
There was a problem hiding this comment.
Good question. So we need to expose the create_* methods in AccountsApi to be used via the SDK. Rather than doing a import in a client as import databricks_cli.accounts.api in a client, we're doing a import here directly.
| @@ -794,3 +794,104 @@ def get_instance_pool(self, instance_pool_id=None, headers=None): | |||
| def list_instance_pools(self, headers=None): | |||
| _data = {} | |||
| return self.client.perform_query('GET', '/instance-pools/list', data=_data, headers=headers) | |||
|
|
|||
|
|
|||
| class AccountsService(object): | |||
There was a problem hiding this comment.
Was this change autogenerated?
There was a problem hiding this comment.
No @andrewmchen, I implemented it. Sorry, should it have been auto-generated somehow?
There was a problem hiding this comment.
As per @andrewmchen - this needs to be inlined in the proto as this is auto-generated.
…t related errors from the Travis CI build. Tested this locally now.
| if account_id is None: | ||
| raise TypeError('Expected account_id') | ||
| endpoint = "/accounts/%s/credentials" % (account_id) | ||
| return self.client.client.perform_query('POST', endpoint, data=json) |
There was a problem hiding this comment.
Why not write these into the client SDK as well?
There was a problem hiding this comment.
I did not see any other POST perform_query methods accepting a JSON for other objects in the client SDK, so I skipped these too.
| help=JsonClickType.help('/api/2.0/accounts/{account_id}/credentials')) | ||
| @debug_option | ||
| @profile_option | ||
| @eat_exceptions |
There was a problem hiding this comment.
This looks foreboding. Does this mean exceptions won't be surfaced?
There was a problem hiding this comment.
Just repeating the pattern from other objects in the CLI.
| @click.option('--json', default=None, type=JsonClickType(), | ||
| help=JsonClickType.help('/api/2.0/accounts/{account_id}/credentials')) | ||
| @debug_option | ||
| @profile_option |
There was a problem hiding this comment.
Just repeating the pattern from other objects in the CLI. I think @profile_option makes the command open to --profile parameter which would correspond to a particular workspace environment.
| if method in self.restful_methods: | ||
| restful_method = method | ||
| if method == 'REST-GET': | ||
| restful_method = 'GET' |
There was a problem hiding this comment.
Why is this necessary? I see you don't typically pass in data anyways. If you don't won't method = GET do the same thing?
There was a problem hiding this comment.
I just tried to keep the restful API separate from the existing API where we try and create the query parameters for GET today. Didn't want to cause an unintended change due to how APIs are being designed now vs earlier.
There was a problem hiding this comment.
@abhinavg6 but data is always empty in AccountsService, therefore REST-GET might be unnecessary
There was a problem hiding this comment.
Guys - Please feel free to fix this after it has been merged. I've no time to look at it and test it now. At this point I don't care if it gets approved or not, because I was trying to do this coz Eng didn't have capacity and CLI is often overlooked for new APIs.
| if method in self.restful_methods: | ||
| restful_method = method | ||
| if method == 'REST-GET': | ||
| restful_method = 'GET' |
There was a problem hiding this comment.
@abhinavg6 but data is always empty in AccountsService, therefore REST-GET might be unnecessary
| def close(self): | ||
| """Close the client""" | ||
| pass | ||
|
|
||
| # helper functions starting here | ||
|
|
||
| def perform_query(self, method, path, data = {}, headers = None): | ||
| """Adding for Accounts API support""" | ||
| path_specific_url = "" | ||
| if path.startswith("/accounts"): |
There was a problem hiding this comment.
@abhinavg6 we might not need this change, because ApiClient shouldn't be aware of different URLs. There's also "Execution Context API", which can be used by this very same ApiClient class without the need of modifying it.
It already has host, username and password, which are part of profile configuration. And most likely we'll have to have one profile for "usual" workspace work like clusters/jobs and another profile for "multiworkspace" setup.
Also, this client could have different "provider function", that is different from databricks_cli.configure.config#provide_api_client - something like provide_multiworkspace_client.
it might also be helpful to add few lines to README.md to describe how to use both modes and how to configure multiworkspace functionality.
There was a problem hiding this comment.
Please feel free to change if this is merged. I'm not doing anything right now.
|
@abhinavg6 It's been a while since the last update. Do you want to reboot or abort this PR? |
This is a PR to add E2/WMS Accounts API support to the Databricks CLI. Support for these high-level constructs has been added as part of this:
Idea is that the objects could also be used via the SDK Client in customer projects. This is one example: https://github.com/abhinavg6/awsdb-workspace-provisioner.