[HDInsight] Support new feature: create kafka cluster with kafka rest proxy #11529
Conversation
aim-for-better
force-pushed
the
SupportKafkaRestProxy
branch
from
aef7ca4 to
028055d
Compare
|
@Juliehzl Could you please help us review this PR? Can we join S162? I added S162 milestone by myself. |
|
@haroldrandom @Juliehzl Is there someone can help review our PR? It's already 3 work days since we submitted the PR. |
|
Bump SDK version needs to update tests I think? |
|
@haroldrandom The SDK is backward compatible. The newer SDK only adds a few properties to support this new feature. So in general it is not necessary to update tests due to SDK version change. |
| arg_group='Customer Managed Key', help='Algorithm identifier for encryption.') | ||
|
|
||
| # Kafka Rest Proxy | ||
| c.argument('kafka_client_group_id', arg_group='Kafka Rest Proxy', |
There was a problem hiding this comment.
Is it possible to provide short name for your argument here?
There was a problem hiding this comment.
Is it possible to provide short name for your argument here?
Hi @Juliehzl , Thanks. This is the shortest name that can explain the meaning. And there are already some properties' name are longer than this one. For example : workernode_data_disk_storage_account_type=None, workernode_data_disk_size=None,
We should consider the meaning firstly, right?
There was a problem hiding this comment.
You can provide options_list=['--kafka-client-group-id', '-k'] to give both descriptive name and short name. (Feel free to find a vaild short name here)
The same for the following one.
| # Kafka Rest Proxy | ||
| c.argument('kafka_client_group_id', arg_group='Kafka Rest Proxy', | ||
| help='The client AAD security group id for Kafka Rest Proxy') | ||
| c.argument('kafka_client_group_name', arg_group='Kafka Rest Proxy', |
There was a problem hiding this comment.
The same
The same with above
We know the newer SDK should be backward compatible, but I think the new SDK may have api version number change, right? Re-recording your tests in hdinsight module based on the new SDK can help double-check the functionality of new SDK and update the request url to make it consistent with the new SDK used. We really recommend you re-record your tests here. |
|
LGTM in general. Please resolve all my comments and if CI agrees I will approve. |
Hi Zunli, I have added a test for the new properties. Thanks |
aim-for-better
force-pushed
the
SupportKafkaRestProxy
branch
from
5c2858a to
22035a7
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi @Juliehzl , how do you like our PR? |
src/azure-cli/azure/cli/command_modules/hdinsight/tests/latest/test_hdinsight_commands.py
Show resolved
Hide resolved
4 participants
This PR is related with swagger PR [HDInsight] Support kafka rest proxy feature
This 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.