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

mount secret volume cli #6223

Merged
merged 21 commits into from
Apr 30, 2023
Merged

Conversation

smzeng
Copy link
Member

@smzeng smzeng commented Apr 19, 2023


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

Related command

az containerapp create
az containerapp update

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

Hi @smzeng,
Since the current milestone time is less than 7 days, this pr will be reviewed in the next milestone.

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

Hi @smzeng,
If you want to release the new extension version.
Please write the description of changes into HISTORY.rst and update setup.py.

@ghost ghost requested a review from yonzhan April 19, 2023 22:19
@ghost ghost added the Auto-Assign Auto assign by bot label Apr 19, 2023
@ghost ghost requested a review from wangzelin007 April 19, 2023 22:19
@yonzhan
Copy link
Collaborator

yonzhan commented Apr 19, 2023

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

@ghost ghost requested a review from yanzhudd April 19, 2023 22:19
@ghost ghost assigned zhoxing-ms Apr 19, 2023
@ghost ghost added the ContainerApp label Apr 19, 2023
@ghost ghost requested review from zhoxing-ms and jsntcy April 19, 2023 22:19
Copy link
Contributor

@xwang971 xwang971 left a comment

Choose a reason for hiding this comment

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

Can we add some explanations on help.py?

@smzeng
Copy link
Member Author

smzeng commented Apr 24, 2023

if you wish to test the changes you can use this demo cli by running the following command, just remember to downgrade back to normal version after :)
az extension add --upgrade --source https://stacyzengcli.blob.core.windows.net/publicclicontainer/containerapp-0.3.28-py2.py3-none-any.whl

example commands/yaml can be found at the end of this file https://microsoft-my.sharepoint.com/:w:/p/stacyzeng/EXw3LayK-BNMh3qa4usyUCAB9VcR-GBsUWlytV25iE6VWg?e=fWcVkr

@smzeng smzeng marked this pull request as draft April 24, 2023 23:40
@smzeng smzeng marked this pull request as ready for review April 26, 2023 17:02
@smzeng
Copy link
Member Author

smzeng commented Apr 26, 2023

@yonzhan hi this is ready to be merged

@smzeng
Copy link
Member Author

smzeng commented Apr 26, 2023

@zhoxing-ms hi, this is ready to be reviewed, hoping to get this merged by the end of the month :)

class ContainerAppMountSecretTest(ScenarioTest):
@AllowLargeResponse(8192)
@ResourceGroupPreparer(location="northcentralus")
@live_only() # encounters 'CannotOverwriteExistingCassetteException' only when run from recording (passes when run live)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please provide specific error message? We can try to see if we can solve this issue

Copy link
Member Author

@smzeng smzeng Apr 27, 2023

Choose a reason for hiding this comment

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

sorry this comment was copied over from the Ingress Sticky Sessions Test (

@live_only() # encounters 'CannotOverwriteExistingCassetteException' only when run from recording (passes when run live)
)
.
ContainerAppMountSecretTest works both live and not live, I have removed @liveonly from mount secret test

Copy link
Member Author

@smzeng smzeng Apr 27, 2023

Choose a reason for hiding this comment

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

The specific error message that is seen by the Ingress Sticky session is and occurs at line 28 (

self.cmd('containerapp env show -n {} -g {}'.format(env, resource_group), checks=[
)

ex = CannotOverwriteExistingCassetteException("Can't overwrite existing cassette ('/home/stacyzeng/azure-cli-extensions/src...d', 'scheme', 'host', 'port', 'path']\nMatchers failed :\n_custom_request_query_matcher - assertion failure :\nNone\n")
    def _polish_bad_errors(ex):
        import json
        try:
            content = json.loads(ex.response.content)
            if 'message' in content:
                detail = content['message']
            elif 'Message' in content:
                detail = content['Message']

            ex = CLIInternalError(detail)
        except Exception:  # pylint: disable=broad-except
            pass
        if no_throw:
            return ex
>       raise ex

src/containerapp/azext_containerapp/_client_factory.py:28:

Copy link
Member Author

Choose a reason for hiding this comment

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

since debugging my take a while, and thie tests in this PR dont have this bug, if possible can this PR first be merged and then fixing this issue be a seperate PR @zhoxing-ms ?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, sounds good~

@zhoxing-ms zhoxing-ms merged commit 3fd86f7 into Azure:main Apr 30, 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.

None yet

5 participants