Add more configurable flags for UC metastore, external locations, and storage credentials. #562
Conversation
|
@adamcain-db Could you take a pass? |
pietern
left a comment
There was a problem hiding this comment.
LGTM
@adamcain-db Will defer approval to you. I'll take care of merge and release when done.
adamcain-db
left a comment
There was a problem hiding this comment.
LGTM. Just some questions about input validation.
Thanks for adding all these flags and also the unit tests!
| data = { | ||
| 'name': name, | ||
| 'comment': comment | ||
| } |
There was a problem hiding this comment.
Looks like we are leaving it completely to the server to validate the input -- e.g., we are not checking that data has required fields like name. I suppose that's reasonable (as we also do not validate JSON inputs), but I wanted to confirm that this is our approach for input validation at the CLI.
There was a problem hiding this comment.
Yes, that's the idea. Instead of replicating the full validation logic, just letting the server do it and then return the error message would make things easier to maintain.
| help='Path URL for the new external location') | ||
| @click.option('--storage-credential-name', default=None, | ||
| help='Name of storage credential to use with new external location') | ||
| @click.option('--read-only/--no-read-only', is_flag=True, default=None, |
There was a problem hiding this comment.
Wow! I'm thrilled that when the data dict has fields like 'read_only': None, the JSON sent has "read_only": null and then the server sees that field omitted. Party on, Garth....
| cred_az_application_id, cred_az_client_secret, | ||
| cred_az_mi_access_connector_id, | ||
| cred_az_mi_id, cred_gcp_sak_email, cred_gcp_sak_private_key_id, | ||
| cred_gcp_sak_private_key): |
There was a problem hiding this comment.
No biggie, but I wonder if this CLI should be validate_storage_credential. It still requires a url, of course, but the CLI name would at least match the API endpoint. Eh, just a thought...
There was a problem hiding this comment.
Yeah, I am not sure why it's named differently, but unfortunately, renaming a command is more trouble than it's worth. If we want to, it should be in a different PR with proper deprecation patterns.
| 'The service account\'s RSA private key.')) | ||
| @click.option('--comment', default=None, | ||
| help='Free-form text description.') | ||
| @create_update_common_options |
Added all configurable flags for create and update operations.
Added unit tests for all CLI commands relevant to the 3 securables.
Added .vscode folder to automatically configure lint and pytest.
Included GCP credentials in preparation for private preview.
Manually tested with a handful of commands.