Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support --token-store, --sas-url-secret, --sas-url-secret-name, --yes for container app auth #6660

Merged
merged 5 commits into from
Aug 21, 2023

Conversation

njuCZ
Copy link
Contributor

@njuCZ njuCZ commented Aug 18, 2023

… for az cotnainerapp auth


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

Related command

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

azure-client-tools-bot-prd bot commented Aug 18, 2023

⚠️Azure CLI Extensions Breaking Change Test
⚠️containerapp
rule cmd_name rule_message suggest_message
⚠️ 1006 - ParaAdd containerapp auth update cmd containerapp auth update added parameter sas_url_secret_name
⚠️ 1006 - ParaAdd containerapp auth update cmd containerapp auth update added parameter sas_url_secret
⚠️ 1006 - ParaAdd containerapp auth update cmd containerapp auth update added parameter token_store
⚠️ 1006 - ParaAdd containerapp auth update cmd containerapp auth update added parameter yes

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

Hi @njuCZ,
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
Collaborator

yonzhan commented Aug 18, 2023

Thank you for your contribution! We will review the pull request and get back to you soon.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Auto-Assign Auto assign by bot label Aug 18, 2023
@njuCZ njuCZ changed the title Support --token_store, --sas-url-secret, --sas-url-secret-name, --yes for container app auth Support --token-store, --sas-url-secret, --sas-url-secret-name, --yes for container app auth Aug 18, 2023
if self.get_argument_sas_url_secret() is not None and self.get_argument_sas_url_secret_name() is not None:
raise ArgumentUsageError('Usage Error: --sas-url-secret and --sas-url-secret-name cannot both be set')
if self.get_argument_sas_url_secret() is not None and not self.get_argument_yes():
msg = 'Configuring --sas-url-secret will add a secret to the containerapp. Are you sure you want to continue?'
Copy link
Contributor

Choose a reason for hiding this comment

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

is it required to add prompt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if yes parameter is not passed, there is prompt to let user confirm it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the same behavior as others. For example: az containerapp microsoft update xxx

@@ -5046,6 +5049,8 @@ def update_auth_config(cmd, resource_group_name, name, set_string=None, enabled=
)

containerapp_auth_decorator.construct_payload()
if containerapp_auth_decorator.get_argument_token_store() and containerapp_auth_decorator.get_argument_sas_url_secret() is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

why not add it in construct_payload()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

set_secrets func is defined in this file, to avoid dependency loop, so place the logic here

Copy link
Contributor

@Greedygre Greedygre Aug 18, 2023

Choose a reason for hiding this comment

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

@Juliehzl
g.custom_command('set', 'set_secrets', exception_handler=ex_handler_factory())
1. set_secrets is a function support command
2. set_secrets will execute ContainerAppClient.create_or_update, I think if it add in construct_payload, maybe will execute twice in some scenario. It should be set in some place which not super() the GA function.

@@ -5046,6 +5049,8 @@ def update_auth_config(cmd, resource_group_name, name, set_string=None, enabled=
)

containerapp_auth_decorator.construct_payload()
if containerapp_auth_decorator.get_argument_token_store() and containerapp_auth_decorator.get_argument_sas_url_secret() is not None:
set_secrets(cmd, name, resource_group_name, secrets=[f"{BLOB_STORAGE_TOKEN_STORE_SECRET_SETTING_NAME}={containerapp_auth_decorator.get_argument_sas_url_secret()}"], no_wait=True, disable_max_length=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

combine the method to depcorator to avoid duplicate usage when moving to GA and refine the logic to be same with facbook/microsoft... commands

Copy link
Contributor

Choose a reason for hiding this comment

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

Will do it in next PR.

@Greedygre
Copy link
Contributor

Hi @zhoxing-ms
Could you please help to review and merge this PR, thanks.

@zhoxing-ms zhoxing-ms merged commit c7c645a into Azure:main Aug 21, 2023
@njuCZ njuCZ deleted the authConfigTokenStore branch August 23, 2023 02:28
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.

5 participants