Conversation
|
This pull request has been linked to Clubhouse Story #67241: [DCS-FM] Enrich the python client for the FM APIs. |
|
This pull request has been linked to Clubhouse Story #63940: [DCS-FM] Implement Fleet Manager API in dataiku-api-client-python. |
FChataigner
left a comment
FChataigner
left a comment
There was a problem hiding this comment.
- the
create_xxx()calls are unusable, there are way too many args, and depending on the cloud only half of them are relevant. A newnew_xxx()would be better, like thenew_recipe()in the DSS api. - the FMCloudCredentials doesn't have any helper to get/set fields. Simple operations like setting the license or changing the credentials of the tenant are hard to do
- cloud tags getting/setting isn't exposed
- missing
__init__.pyin thefmmodule - the
get_things()(plural) methods should belist_things()instead - no way to get the default vpc/subnet when creating networks, or to leave the values empty and let the backend fill them with the defaults
- no helper on instance templates, not even to add setup actions (so the part to add setup actions isn't discoverable)
- not all the setup actions have their wrapper
- the instance has no helper to get/set fields, and should definitely have different classes depending on the cloud
- no api to make snapshots of instances (but to be fair the call isn't in the backend either)
dataikuapi/fmclient.py
Outdated
|
|
||
| def get_cloud_credentials(self): | ||
| """ | ||
| Get Cloud Credential |
There was a problem hiding this comment.
pluralize credential ?
dataikuapi/fmclient.py
Outdated
| class FMClient(object): | ||
| """Entry point for the FM API client""" | ||
|
|
||
| def __init__(self, host, api_key_id, api_key_secret, tenant_id, extra_headers = None): |
There was a problem hiding this comment.
the tenant_id could have a default value main since that's what MONOTENANT setups have
|
I've updated the PR, but left the snapshots and getting the default vpc/subnet when creating networks out of the scope because the public api is lacking for now. |
FChataigner
left a comment
FChataigner
left a comment
There was a problem hiding this comment.
- not too fond of the methods
set_...()that do asave()internally. I'd rather have the method returnselfso that you can chain asave()if you want - no way to guess the image id for creation
| self.api_key_id = api_key_id | ||
| self.api_key_secret = api_key_secret | ||
| self.host = host | ||
| self.__tenant_id = tenant_id |
There was a problem hiding this comment.
An old habit, __ for private variables vs _ for protected variables.
I can update it to a single underscore if you prefer to
dataikuapi/fmclient.py
Outdated
| creds = self._perform_tenant_json("GET", "/cloud-credentials") | ||
| return FMCloudCredentials(self, creds) | ||
|
|
||
| def get_cloud_tags(self, tenant_id): |
There was a problem hiding this comment.
why pass another tenant_id here? and not the FMClient's one?
dataikuapi/fm/tenant.py
Outdated
| self.cloud_credentials["awsCMKId"] = cmk_key_id | ||
| self.save() | ||
|
|
||
| def set_static_license(self, license_file=None, license_string=None): |
There was a problem hiding this comment.
license_file is ambiguous. Use license_file_path instead, or do a type check to assess whether the object is a file-like or not
dataikuapi/fm/tenant.py
Outdated
| raise Exception("Key already exists") | ||
| self.cloud_tags[key] = value | ||
|
|
||
| def update_tag(self, key, new_key=None, new_value=None): |
There was a problem hiding this comment.
I'd move the "key renaming" part to another method, call the method set_tag() and not complain if the tag doesn't already exist
| ) | ||
|
|
||
|
|
||
| class FMCloudTags(object): |
There was a problem hiding this comment.
helpers for getting/setting the tags seem a bit overkill. A property to get the tag dict and let the user update it would suffice
dataikuapi/fm/virtualnetworks.py
Outdated
| self.data["awsAutoCreateSecurityGroups"] = True | ||
| return self | ||
|
|
||
| def with_aws_security_groups(self, aws_security_groups): |
There was a problem hiding this comment.
maybe a signature with_aws_security_groups(self, *aws_security_groups): would be friendlier
| self.data["setupActions"] = setup_actions | ||
| return self | ||
|
|
||
| def with_license(self, license): |
dataikuapi/fmclient.py
Outdated
| self.host = host | ||
| if cloud not in ["AWS", "Azure"]: | ||
| raise ValueError("cloud should be either \"AWS\" or \"Azure\"") | ||
| self.cloud = cloud |
There was a problem hiding this comment.
Careful, I seem to remember that AZURE should be in capital here. Can you check ?
dataikuapi/fm/instances.py
Outdated
| """ | ||
| self.data["cloudInstanceType"] = cloud_instance_type | ||
|
|
||
| def set_data_volume_options(self, data_volume_type=None, data_volume_size=None, data_volume_size_max=None, data_volume_IOPS=None, data_volume_encryption=None, data_volume_encryption_key=None): |
There was a problem hiding this comment.
Is it usable like this for both Azure and AWS, or should a different call be devised ?
4 participants
[ch63940]