-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add acc sgx addon cli support #2253
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
Conversation
|
If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR: |
src/aks-preview/azext_aks_preview/tests/latest/test_aks_commands.py
Outdated
Show resolved
Hide resolved
|
aks |
|
hi @arrownj could you pls review this PR? thanks |
| self.check('[0].roleDefinitionName', 'Contributor') | ||
| ]) | ||
|
|
||
| @live_only() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any special case to mark it as live_only ? Same question for below cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will fail at "az login" without live-only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any advice how to fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I'm getting this error with recordings:
vcr.errors.CannotOverwriteExistingCassetteException: Can't overwrite existing cassette ('/home/vsts/work/1/s/src/aks-preview/azext_aks_preview/tests/latest/recordings/test_aks_enable_addons_confcom_addon.yaml') in your current record mode ('once').
No match for the request (<Request (GET) https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/clitest000001/providers/Microsoft.ContainerService/managedClusters/cliakstest000002?api-version=2020-06-01>) was found.
Found 1 similar requests with 0 different matcher(s) :
1 - (<Request (GET) https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/clitest000001/providers/Microsoft.ContainerService/managedClusters/cliakstest000002?api-version=2020-06-01>).
Matchers succeeded : ['method', 'scheme', 'host', 'port', 'path', '_custom_request_query_matcher']
Matchers failed :
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking two failing tests as live-only for now since I need to merge this PR asap. They passed locally for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bowang-666, not sure why the tests required az login ? If actually needed, the test framework also provides a replacer function to handle this case.
Are the failed cases new added ? It's better not to skip any CI failure for there may be potential bugs we didn't find. If you need to use the extension in a short time, you can build a wheel file in your local environment(just cd aks-preview, and run python ./setup.py bdist_wheel). For the normal release, let's follow the release process to get CI passed first.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not need "az login" now, the only issue now is "Can't overwrite existing cassette" error in test pipeline. I need to release a new version of aks-preview for ACC team asap, but I also agree we should investigate this further. How about I open a bug to track this issue separately? The tests passed lively so I'm confident the functionality is working.
arrownj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's submit another issue to trace the test problem.
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally?For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update
src/index.jsonautomatically.The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify
src/index.json.