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

Containerapp 0.3.10 Release #5241

Merged
merged 18 commits into from
Aug 25, 2022
Merged

Conversation

StrawnSC
Copy link
Contributor


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.
The precondition is to put your code inside this repository and upgrade the version in the pull request but do not modify src/index.json.

@ghost ghost requested a review from yonzhan August 16, 2022 18:13
@ghost ghost added the Auto-Assign Auto assign by bot label Aug 16, 2022
@ghost ghost requested a review from wangzelin007 August 16, 2022 18:13
@ghost ghost assigned zhoxing-ms Aug 16, 2022
@ghost ghost added this to the Aug 2022 (2022-09-06) milestone Aug 16, 2022
@ghost ghost added the ContainerApp label Aug 16, 2022
@ghost ghost requested review from zhoxing-ms and jsntcy August 16, 2022 18:13
@yonzhan
Copy link
Collaborator

yonzhan commented Aug 16, 2022

Containerapp

@StrawnSC StrawnSC marked this pull request as ready for review August 22, 2022 20:06
@panchagnula
Copy link
Contributor

@StrawnSC / @haroonf when is this version going to be ready for release? Seems like it missing some other changes we discussed should be part of the release?

@StrawnSC
Copy link
Contributor Author

@StrawnSC / @haroonf when is this version going to be ready for release? Seems like it missing some other changes we discussed should be part of the release?

@panchagnula is this the change you were talking about: calvinsID#145 (Rune's fix for the --location in az containerapp hostname list)?
The only other outstanding change is the "no dockerfile" feature, but I thought we wanted to give some time for @anthonychu to try it out. I wouldn't be opposed to merging that with this release though

@panchagnula
Copy link
Contributor

@StrawnSC / @haroonf when is this version going to be ready for release? Seems like it missing some other changes we discussed should be part of the release?

@panchagnula is this the change you were talking about: calvinsID#145 (Rune's fix for the --location in az containerapp hostname list)? The only other outstanding change is the "no dockerfile" feature, but I thought we wanted to give some time for @anthonychu to try it out. I wouldn't be opposed to merging that with this release though

if we need to wait on testing/ sign-off from Anthony that is ok - but we need to get the regression fix deployed soon, so lets prioritize that accordingly. Thanks!

@panchagnula
Copy link
Contributor

@zhoxing-ms could you review this today & share your initial comments!

@AllowLargeResponse(8192)
@ResourceGroupPreparer(location="northeurope")
@live_only() # encounters 'CannotOverwriteExistingCassetteException' only when run from recording (passes when run live)
def test_containerapp_anonymous_registry(self, resource_group):
Copy link
Contributor

Choose a reason for hiding this comment

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

In general can we work with core-cli to figure out getting these tests run without live mode. Given extension tests don't run in nightly or release builds, or PR builds, these don't help until a dev runs all tests locally.
The containerapp-compose is supposed to have running these kind of tests, using a monkey patch of some sort, can we do something similar?

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem we bumped into was the generated name for the log analytics workspace.

We worked around this by adding a hidden/suppressed parameter and then using the self.create_random_name to pass in a known value (known to the test harness) for the name of the log analytics workspace.

This required a monkey patch of _generate_log_analytics_workspace_name by https://github.com/Azure/azure-cli-extensions/blob/main/src/containerapp-compose/azext_containerapp_compose/_monkey_patch.py#L57-L60

I'd be happy to grab some time to pair to see if that'd be helpful for your tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @smurawski - given this a create command that can take an existing log-analytics as an input I am curious why this failing. but to your point yes, happy to engage with you as needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@panchagnula - in this test (see line 575), it's doing a containerapp env create which generates a log analytics workspace with a random name. This will fail on any attempt to replay the recorded test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion @smurawski! I'm testing this right now. Thankfully, we can pass in log analytics workspace names to az containerapp env create without needing to add hidden parameters to any of our commands, so this might be an easy fix for us

Copy link
Contributor

@panchagnula panchagnula left a comment

Choose a reason for hiding this comment

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

Please update the validation logic of the test. Create a work item at the min to investigate & work with core-cli how to make the "live only" tests (espcially for not "up") work as true scenario tests.

@StrawnSC StrawnSC requested a review from panchagnula August 25, 2022 00:42
@StrawnSC
Copy link
Contributor Author

@panchagnula I made the change to testing validation you mentioned above. The fix @smurawski suggested didn't work for all our tests (but I changed it for the ones it worked for) so I went ahead and made a work item to investigate this further so it won't block this release

@panchagnula
Copy link
Contributor

@panchagnula I made the change to testing validation you mentioned above. The fix @smurawski suggested didn't work for all our tests (but I changed it for the ones it worked for) so I went ahead and made a work item to investigate this further so it won't block this release

Thanks a lot @StrawnSC , good to see the Diff show more test cases are not live mode anymore. For UP , we need to figure out a better way to make these not live. We can work with core-cli as well during our investigation. These might be tests we need more eyes, so we will still go our manual test pass route.

Copy link
Contributor

@panchagnula panchagnula left a comment

Choose a reason for hiding this comment

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

Approved.

@zhoxing-ms
Copy link
Contributor

@StrawnSC Could you please address those CI issues?

@zhoxing-ms zhoxing-ms merged commit 60f136e into Azure:main Aug 25, 2022
@azclibot
Copy link
Collaborator

[Release] Update index.json for extension [ containerapp ] : https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1805860&view=results

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.

7 participants