-
Notifications
You must be signed in to change notification settings - Fork 872
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
Adding an azure-nextgen example for placing app service behind vnet #909
Conversation
azure-nextgen-ts-webapp-privateendpoint-vnet-injection/index.ts
Outdated
Show resolved
Hide resolved
azure-nextgen-ts-webapp-privateendpoint-vnet-injection/index.ts
Outdated
Show resolved
Hide resolved
azure-nextgen-ts-webapp-privateendpoint-vnet-injection/package.json
Outdated
Show resolved
Hide resolved
import * as random from "@pulumi/random"; | ||
|
||
const config = new pulumi.Config(); | ||
const resourceGroupNameParam = config.require("resourceGroupNameParam"); |
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.
Why require the resource group name? Should we add a default? Otherwise, we need add a command to set it to the readme.
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.
Added a default and cleaned up a lot of the parameter cruft from the arm template conversion.
@@ -0,0 +1,174 @@ | |||
// Copyright 2016-2021, Pulumi Corporation. All rights reserved. | |||
|
|||
import * as azure_nextgen from "@pulumi/azure-nextgen"; |
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.
Other examples use a different import style (e.g. https://github.com/pulumi/examples/blob/master/azure-nextgen-ts-aks/index.ts#L7-L8). Should we do the same here?
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.
Fixed.
azure-nextgen-ts-webapp-privateendpoint-vnet-injection/index.ts
Outdated
Show resolved
Hide resolved
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.
This looks great! A couple of small questions.
It would be nice to deploy a demo app to show that this actually works, but I understand that's beyond the scope for now.
What's our plan for porting to other languages? Manually or via PCL?
azure-nextgen-ts-webapp-privateendpoint-vnet-injection/index.ts
Outdated
Show resolved
Hide resolved
// Copyright 2016-2021, Pulumi Corporation. All rights reserved. | ||
|
||
// pvt DNS zone and virtual network link are only available on this version | ||
import * as pvtnetwork from "@pulumi/azure-nextgen/network/v20180901"; |
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.
Does it have to be this old version? Have you tried latest
for all?
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.
Latest works.
resourceGroupName: resourceGroup.name, | ||
serverFarmId: serverfarm.id, | ||
siteConfig: { | ||
ftpsState: web.FtpsState.AllAllowed, |
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.
Do you know if we need this setting?
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.
No don't think so, came from the arm template.
Co-authored-by: Mikhail Shilkov <github@mikhail.io>
I was playing around with something but didn't get it to work yet. Will update the PR with a clear demonstration of the front-end -> backend interaction via the private endpoint as soon as I get it working.
Most of this is convertible with PCL now with some cleanup. Will look to get that out shortly. |
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.
Looks great! Just one remark with a possible simplification.
|
||
const serverfarm = new web.AppServicePlan("serverfarm", { | ||
kind: "app", | ||
location: location, |
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.
If you update to 0.6.1, you can omit location
property everywhere by setting the azure-nextgen:location
config 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.
Done
Adding an azure-nextgen example for placing app service behind vnet
Fixes pulumi/pulumi-azure#263