Skip to content

[containerapp] az containerapp connected-env: Sopport dapr-component/storage/certificate#6727

Merged
yanzhudd merged 14 commits into
Azure:mainfrom
Greedygre:xinyu/move_dapr_storage_certificate
Sep 18, 2023
Merged

[containerapp] az containerapp connected-env: Sopport dapr-component/storage/certificate#6727
yanzhudd merged 14 commits into
Azure:mainfrom
Greedygre:xinyu/move_dapr_storage_certificate

Conversation

@Greedygre
Copy link
Copy Markdown
Contributor


This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

az containerapp connected-env certificate delete/list/upload
az containerapp connected-env dapr-component list/remove/set/show
az containerapp connected-env storage list/remove/set/show

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

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.json automatically.
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.

@azure-client-tools-bot-prd
Copy link
Copy Markdown

azure-client-tools-bot-prd Bot commented Sep 6, 2023

⚠️Azure CLI Extensions Breaking Change Test
⚠️containerapp
rule cmd_name rule_message suggest_message
⚠️ 1011 - SubgroupAdd containerapp connected-env certificate sub group containerapp connected-env certificate added
⚠️ 1011 - SubgroupAdd containerapp connected-env dapr-component sub group containerapp connected-env dapr-component added
⚠️ 1011 - SubgroupAdd containerapp connected-env storage sub group containerapp connected-env storage added

@azure-client-tools-bot-prd
Copy link
Copy Markdown

Hi @Greedygre,
Please write the description of changes which can be perceived by customers into HISTORY.rst.
If you want to release a new extension version, please update the version in setup.py as well.

@yonzhan
Copy link
Copy Markdown
Collaborator

yonzhan commented Sep 6, 2023

containerapp

Comment thread src/containerapp/azext_containerapp/_clients.py Outdated
@Greedygre Greedygre marked this pull request as ready for review September 7, 2023 05:21
Comment thread src/containerapp/azext_containerapp/_clients.py
Comment thread src/containerapp/azext_containerapp/_help.py
Comment thread src/containerapp/azext_containerapp/_params.py Outdated
Comment thread src/containerapp/azext_containerapp/custom.py Outdated
Comment thread src/containerapp/azext_containerapp/custom.py Outdated
Comment thread src/containerapp/azext_containerapp/custom.py Outdated
Comment thread src/containerapp/azext_containerapp/custom.py Outdated
@Greedygre Greedygre changed the title {containerapp} move connected dapr-component storage certificate [containerapp] az containerapp connected-env: Sopport dapr-component/storage/certificate Sep 14, 2023
Comment thread src/containerapp/azext_containerapp/_clients.py Outdated
@Greedygre
Copy link
Copy Markdown
Contributor Author

Greedygre commented Sep 14, 2023

Hi @zhoxing-ms
This PR has been reviewed and approved by zunli. Could you help to review and merge this PR? Thanks.

Comment on lines -340 to -342
c.argument('dapr_app_id', help="The Dapr app ID.")
c.argument('dapr_app_port', help="The port of your app.")
c.argument('dapr_app_protocol', help="Tell Dapr which protocol your application is using. Allowed values: grpc, http.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask if these parameters are not available anymore after this change? will this change make a breaking change for customers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These parameters are not supported before, they are unused.
So it will not be a breaking change.

Comment thread src/containerapp/azext_containerapp/_params.py
c.argument('prompt', options_list=['--show-prompt'], action='store_true', help='Show prompt to upload an existing certificate.')

with self.argument_context('containerapp connected-env certificate list') as c:
c.argument('name', id_part=None)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add help message for this parameter.

Copy link
Copy Markdown
Contributor Author

@Greedygre Greedygre Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name's help currently come from argument group containerapp connected-env, no need to add help for name again in sub command group.
But argument name in sub command has setting id_part=None, which is different with containerapp connected-env.

@ResourceGroupPreparer(location=TEST_LOCATION, random_name_length=15)
@ConnectedClusterPreparer(location=TEST_LOCATION)
def test_containerappjob_preview_environment_type(self, resource_group, infra_cluster, connected_cluster_name):
self.cmd('configure --defaults location={}'.format(TEST_LOCATION))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask why need to config the default value of location intead of inputting manually?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is because we need all test resources created in same region, and we have vendor to run tests in all regions one by one.
In core, we have this PR to avoid impact others' services: {containerapp} add random config directory for test by Greedygre · Pull Request #27382 · Azure/azure-cli (github.com).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add random config directory in next PR in extension, consisting with core.

@yanzhudd yanzhudd merged commit 954e4dd into Azure:main Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Auto-Assign Auto assign by bot ContainerApp

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants