Network analytics ingest #7540
Network analytics ingest #7540
Conversation
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| network-analytics data-product ingest | cmd network-analytics data-product ingest added |
azure-client-tools-bot-prd
bot
added this to the May 2024 (2024-05-21)❗ Breaking change release ❗ milestone
|
Hi @sabuqamar-ms, |
|
Hi @sabuqamar-ms, |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
requested review from
yonzhan,
jsntcy,
kairu-ms and
necusjz
microsoft-github-policy-service
bot
added
customer-reported
|
| raise InvalidArgumentValueError(err_msg) | ||
|
|
||
| def get_storage_container(self, secret): | ||
| result = secret.split("?", 1) |
There was a problem hiding this comment.
The keyvault is within the customers subscription so its possible for them to update the secret with a non SAS value. Not likely but possible. We should check if the secret is a valid SAS url e.g. contains ? or contains more than one ?, and error accordingly. Perhaps there is a way to validate a SAS URL before using it?
There was a problem hiding this comment.
The container client will throw an exception if the container instance could not be created (whether from invalid container URL or invalid credentials). If you think we should handle that error, we can discuss that. However, I think it is sufficient.
| DATA_PRODUCT_ARM_ID = "/subscriptions/{}/resourceGroups/{}/providers/Microsoft.NetworkAnalytics/dataProducts/{}" | ||
| API_VERSION = "2023-11-15" | ||
| KEYVAULT_NAME = "input-storage-sas" | ||
| KEYVAULT_URI = "https://aoi-{}-kv.vault.azure.net/" |
There was a problem hiding this comment.
You can define this with a named variable to replace:
| KEYVAULT_URI = "https://aoi-{}-kv.vault.azure.net/" | |
| KEYVAULT_URI = "https://aoi-{unique_id}-kv.vault.azure.net/" |
This provides context as to what the replaced value should be.
There was a problem hiding this comment.
Sure. Edit: I don't think this is valid syntax in python
| return result | ||
|
|
||
| class DataProductsIngest(object): | ||
| DATA_PRODUCT_ARM_ID = "/subscriptions/{}/resourceGroups/{}/providers/Microsoft.NetworkAnalytics/dataProducts/{}" |
There was a problem hiding this comment.
Please provide names for the variables to be replaced as mentioned in my comment about the key vault URI.
There was a problem hiding this comment.
Sure. Edit: I don't this that syntax is valid in python
|
|
||
| class DataProductsIngest(object): | ||
| DATA_PRODUCT_ARM_ID = "/subscriptions/{}/resourceGroups/{}/providers/Microsoft.NetworkAnalytics/dataProducts/{}" | ||
| API_VERSION = "2023-11-15" |
There was a problem hiding this comment.
We currently only have one API version but all resources end up with multiple. Are there examples of supporting multiple API versions for a resource in other CLI commands, and if so, is there a way for us to support that upfront?
There was a problem hiding this comment.
I first decided to hard code the API version as all the other commands in our extension have it hard coded, however, I have learnt since that the code is autogenerated and is overwritten if any changes occur. You are right, I have changed it so that it pulls the latest version
| _args_schema.data_type = AAZStrArg( | ||
| options=["--data-type"], | ||
| arg_group="Body", | ||
| help="Data Type.", |
There was a problem hiding this comment.
This needs to be more explanatory.
| help="Data Type.", | |
| help="The name of the data type into which you wish to ingest the specified file(s).", |
| data = open(self.source, "rb") | ||
| storage_container.upload_blob(name=blob_name, data=data, overwrite=True) | ||
| except: | ||
| err_msg = "The source directory provided is invalid or cannot be accessed." |
There was a problem hiding this comment.
"Invalid" implies that a directory can be valid or invalid but doesnt state what a "valid" directory is.
There was a problem hiding this comment.
I have moved this to argument validation and have changed the error message
| def __call__(self, *args, **kwargs): | ||
| data_product = self.get_data_product() | ||
| secret = self.get_key_vault_secret(data_product) | ||
| self.upload_file(secret) |
There was a problem hiding this comment.
Im a bit confused. We accept --srcdir but then it looks like we only update a single file. My other comments suggests that we should be more explicit about it being a single file anyway but just wanted to highlight this 😸
There was a problem hiding this comment.
As per our design meeting, I have changed the flag to --source and implemented as appropriate error message
| class DataProductsIngest(object): | ||
| DATA_PRODUCT_ARM_ID = "/subscriptions/{}/resourceGroups/{}/providers/Microsoft.NetworkAnalytics/dataProducts/{}" | ||
| API_VERSION = "2023-11-15" | ||
| KEYVAULT_NAME = "input-storage-sas" |
There was a problem hiding this comment.
| KEYVAULT_NAME = "input-storage-sas" | |
| INPUT_STORAGE_ACCOUNT_SAS_URL_SECRET_NAME = "input-storage-sas" |
| keyvault_client = data_plane_azure_keyvault_secret_client(self.ctx.cli_ctx, command_args) | ||
| try: | ||
| secret = keyvault_client.get_secret(name=self.KEYVAULT_NAME) | ||
| except ClientAuthenticationError: |
There was a problem hiding this comment.
The key vault exists int he customers subscription, so its entirely possible for them to rename the managed resources resource group, key vault and/or secret. I assume this would break any management of that resource by the data product itself, but it is possible. So we should probably add some error handling that returns a helpful message if the expected resource group, key vault or secret does not exist.
|
|
||
| # pylint: skip-file | ||
| # flake8: noqa | ||
|
|
There was a problem hiding this comment.
I think we want linting wherever we can get it to keep with best practice.
| name = namespace.data_product_name | ||
| length = len(name) | ||
|
|
||
| pattern = "^[a-z][a-z0-9]*$" |
There was a problem hiding this comment.
Will "^[a-z][a-z0-9]{2-62}*$" remove the need for the length checks?
| pattern = "^[a-z][a-z0-9]*$" | ||
| match = re.match(pattern, name) | ||
| if not match or length < 3 or length > 63: | ||
| raise InvalidArgumentValueError("Invalid data product name. Please provide a data product name constisting of letter and/or numbers only, ") |
There was a problem hiding this comment.
Typo: constisting --> consisting.
Also the error message isn't accurate because the accepted letters must be lower-case, and the name must start with a letter, and there is the 3-63 length limit....
| import re | ||
| import os | ||
|
|
||
| logger = get_logger(__name__) |
There was a problem hiding this comment.
Why do you need to create a logger since your code does not use it?
wangzelin007
modified the milestones:
May 2024 (2024-05-21)❗ Breaking change release ❗,
Backlog
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally? (pip install wheel==0.30.0required)For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update
src/index.jsonautomatically.You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify
src/index.json.