[HDInsight] - Support ESP, BYOK and update tags #8294
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
idear1203
force-pushed
the
hdi_support_esp_byok_update_tags
branch
2 times, most recently
from
b4b9723
to
ca57776
Compare
idear1203
force-pushed
the
hdi_support_esp_byok_update_tags
branch
from
ca57776
to
2037408
Compare
| key=s.storage_account_key, | ||
| container=s.container, | ||
| is_default=False | ||
| ) | ||
| for s in additional_storage_accounts | ||
| ] | ||
|
|
||
| cluster_identity = build_identities_info(assign_identity) | ||
| msi_resource_id = assign_identity and next((x for x in assign_identity if x != MSI_LOCAL_ID), None) |
There was a problem hiding this comment.
2 things:
- based on the parameter definition above,
assign_identityis an array, wonder why we only take the first one here? - the validator has ensured system assigned identity will not be possible, i suggest you can simplify the code here and below by not thinking about
MSI_LOCAL_ID; however if system assigned identity will be possible in possible, then we can keep the code here
There was a problem hiding this comment.
Good Catch!
- In the future(this will happen very soon), we will accept more than one assign identities. One is used here while the other one is used for storage. So the parameter should be an array. After all, in this change, array is not necessary. If you feel this is not good, I can change it:)
- I've simplified the code to take the first element of
assign_identitydirectly.
| @@ -14,7 +14,7 @@ | |||
| logger.warn("Wheel is not available, disabling bdist_wheel hook") | |||
| cmdclass = {} | |||
|
|
|||
| VERSION = "0.2.0" | |||
| VERSION = "0.3.0" | |||
There was a problem hiding this comment.
please add the dependency of azure-mgmt-storage
There was a problem hiding this comment.
Added dependency of azure-mgmt-storage and azure-mgmt-network. But I am curious why we have to add dependency here? After all the module seems to work well without adding these dependencies.
idear1203
force-pushed
the
hdi_support_esp_byok_update_tags
branch
2 times, most recently
from
d0abf4b
to
8ae0c38
Compare
idear1203
force-pushed
the
hdi_support_esp_byok_update_tags
branch
from
8ae0c38
to
b023837
Compare
yugangw-msft
approved these changes
Jan 22, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note
Change
create,application create: Removed the--virtual-networkand--subnet-nameparameters.create: Change the--storage-accountto accept name or id of storage account instead of blob endpoints.create: added the--vnet-nameand--subnet-nameparameters.create: added support for Enterprise Security Package and disk encryptionrotate-disk-encryption-keycommandupdatecommandThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.