-
Notifications
You must be signed in to change notification settings - Fork 198
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
Container App Service Target refactor #1989
Conversation
@wbreza Given that this changes the implementation of the deployment mechanism ACA and is a user-facing change, maybe we should avoid calling it a refactor? Can we add additional end-to-end feature details to either #1972 or #1935 that detail the change? That will definitely help speed the review along. I would love to make sure everyone is directionally in-sync before diving deeper into deeper code implementation. |
I have linked the mentioned issues. This change is directional inline with #1935 which will automatically address #1662, #1336 & #1720 |
a16d22d
to
10a21fa
Compare
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.
LGTM.
Since we are off the bicep deployment plan, we should probably remove the module
property from the azure.yaml
schema and then update our ToDo apps to not have seperate web
and api
modules (instead doing everything in main.bicep) since it feels like that would be more idiomatic with this new approach.
Happy for that to happen as a follow up.
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.
Directionally, I think we're getting closer, and I do love the changes here. However, implementation-wise, I don't think we've removed the dependency on requiring a bicep declaration for the Microsoft.App/containerApps
resource. I don't think this is inline with the strategy outlined by Container Apps, based on the deployment mechanisms they suggest. The following comments may eludicate why this is still problematic:
I've looked at the implementation in service_target_containerapp.go
and container_app.go
in detail. One question that emerges that I'm still curious about, is how application configuration (which is environment variables set on the Container App resource) is handled?
Can you clarify:
What does an end-to-end azd application with these changes look like? Does it end up in-practice that we would have the user create a container app resource with a dummy image? If so, we haven't fixed the issue around the dichotomy of a container app being both an ARM resource and a deployment target raised in #1935. I would also like to highlight an ask poised by a community member previously. I think this doesn't remove the requirement for a dummy image. The dummy image mechanism then causes image resets in CI unless an environment variable is set correctly that references the correct image. For me, this was the crux of the problem with the bicep
deployment mechanism, so I want us to hold off until we think about how this can be addressed (either in-scope or separately for this PR).
No, we are not removing the requirement of specifying the Also, it is currently not possible to create a container app resource without specifying an image. Even if you provision manually through the Azure portal there is a checkbox to Without checking this checkbox you need to enter the container details.
It's handled similar to most other service targets through
|
I think we should keep the Agree that we should update our todo templates to remove any old artifacts that are no longer needed. |
But it doesn't do anything for us today, so it feels confusing to expose it in |
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.
LGTM. We spoke offline and the current implementation is an overall improvement. We will revisit some of other raised issues in the near future.
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash:
pwsh:
WindowsPowerShell install
MSI install
Standalone Binary
MSIContainer
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference (preview)
|
Details
This update brings the implementation of container apps inline with our other service targets. Previously ACA apps performed an additional
provision
operation to perform container image modifications during deployment.This additional internal
provision
step introduced additional complexity and also made it difficult to support other infra providers.This update performs the following:
In Scope
Single Revision Mode
Multiple Revision Mode
Out of Scope
Breaking Change
This should also be treated as a breaking change.
provision
step.Workaround
main
module of the azd deployment.Resolves Issues
azd env refresh
loses state #1720