Implement Azure Secrets Manager integration #654
Conversation
strickvl
added
enhancement
…ure-secrets-manager
…nml-io/zenml into feature/ENG-900-azure-secrets-manager
safoinme
left a comment
safoinme
left a comment
There was a problem hiding this comment.
🚀 Let's Go Azure Secrets manager
| secret_name: the name of the secret to delete""" | ||
| self._ensure_client_connected(self.key_vault_name) | ||
|
|
||
| # Go through all GCP secrets and delete the ones with the secret_name |
There was a problem hiding this comment.
Just a typing comment
# Go through all Azure secrets and delete the ones with the secret_name
AlexejPenner
left a comment
AlexejPenner
left a comment
There was a problem hiding this comment.
Just remembered one small topic that we might need to adress somewhere
…ure-secrets-manager
…ure-secrets-manager
htahir1
left a comment
htahir1
left a comment
There was a problem hiding this comment.
Left a few comments. Otherwise looks great! 💯
|
|
||
| for secret_property in self.CLIENT.list_properties_of_secrets(): | ||
| response = self.CLIENT.get_secret(secret_property.name) | ||
| tags = response.properties.tags |
There was a problem hiding this comment.
Is it ensured that properties will exist?
There was a problem hiding this comment.
No, but that's why we have the check on the next line. It's either a list, or None.
|
|
||
| secret_contents[secret_key] = response.value | ||
|
|
||
| zenml_schema_name = response.properties.tags.get( |
There was a problem hiding this comment.
I guess this can be just tags. ?
| if not secret_contents: | ||
| raise RuntimeError(f"No secrets found within the {secret_name}") | ||
|
|
||
| secret_contents["name"] = secret_name |
There was a problem hiding this comment.
What is the key name already exists here? Is it possible?
There was a problem hiding this comment.
It's currently possible, but I've just added a check for that in the CLI.
There was a problem hiding this comment.
I would still throw an exception here if it somehow exists
| secret_property.name, | ||
| tags.get(ZENML_GROUP_KEY), | ||
| ) | ||
| self.CLIENT.begin_delete_secret(secret_property.name).result() |
There was a problem hiding this comment.
What does .result() do. Should we capture it and log it out?
There was a problem hiding this comment.
This method returns a 'poller' (see docs). This would be useful for us if we wanted to wait until we'd confirmed that the deletion process had completed. But we decided that for our CLI this isn't important and we don't need to wait after making this call.
…ure-secrets-manager
| parsed_args = parse_unknown_options(args, expand_args=True) | ||
| except AssertionError as e: | ||
| error(str(e)) | ||
| return |
There was a problem hiding this comment.
Was this return intentionally taken out?
There was a problem hiding this comment.
Yes. The error raises an exception already, so it's a redundant return. I know elsewhere in the codebase we've been removing these redundant returns so I saw it here and removed it.
We added the Azure Secrets Manager integration and updated docs as appropriate.
Pre-requisites
Please ensure you have done the following:
Types of changes