-
Notifications
You must be signed in to change notification settings - Fork 672
Changing default SKU for App Service Plan to P0V3 #9280
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
Changes from 2 commits
5fb3fa7
219ec08
7fc2f16
09ca3c6
d2a0830
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -214,7 +214,7 @@ public void BuildWebSite(AzureResourceInfrastructure infra) | |
AppServicePlanId = appServicePlanParameter, | ||
SiteConfig = new SiteConfigProperties() | ||
{ | ||
LinuxFxVersion = BicepFunction.Interpolate($"DOCKER|{containerImage}"), | ||
LinuxFxVersion = "SITECONTAINERS", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confused by this change. Why do we need sidecars? Or is this just the new way of using containers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes this is what we want to do going fwd instead of overloading the linuxFX version. There is no sidecar being configured here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the port hardcoded to 8080? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed setting the port as it is inferred by the platform. Validated with the changes in fork code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this need to be set to the internal port of the http endpoint? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When TargetPort is not specified, our container orchestrator can infer the exposed PORT in the container image and use it for serving the http traffic. So, explicitly mentioning TargetPort is not required. |
||
AcrUserManagedIdentityId = acrClientIdParameter, | ||
UseManagedIdentityCreds = true, | ||
AppSettings = [] | ||
|
@@ -226,6 +226,18 @@ public void BuildWebSite(AzureResourceInfrastructure infra) | |
}, | ||
}; | ||
|
||
var mainContainer = new SiteContainer("mainContainer") | ||
davidfowl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
Parent = webSite, | ||
Name = "main", | ||
Image = containerImage, | ||
AuthType = SiteContainerAuthType.UserAssigned, | ||
UserManagedIdentityClientId = acrClientIdParameter, | ||
IsMain = true | ||
}; | ||
|
||
infra.Add(mainContainer); | ||
|
||
foreach (var kv in EnvironmentVariables) | ||
{ | ||
var (val, secretType) = ProcessValue(kv.Value); | ||
|
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.
I'm having second thoughts about this one.