-
Notifications
You must be signed in to change notification settings - Fork 89
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
Azure App Service PAAS deployment #65
Conversation
Closes #51 |
@@ -81,6 +81,12 @@ | |||
"dataType": "bool", | |||
"defaultValue": "false", | |||
"description": "use NPM instead of default Yarn for JS package management" | |||
}, | |||
"Azure": { |
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.
How about introducing a choice
parameter, say "Deploy" with two options for now: Docker
and Azure
- both options do not have much sense if combined
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.
That makes sense to me as well.
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.
Agreed. I've made that change.
Content/arm-template.json
Outdated
"sku": { "name": "F1" }, | ||
"name": "[variables('appServicePlan')]", | ||
"apiVersion": "2016-09-01", | ||
"location": "West Europe" |
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 have no experience with ARM but wondering whether all of the options here, like location should be hard-coded?
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.
The SKU and Location can both be parameterised (the Location definitely). @theprash The script is already using the Region
type, which contains consts for all Azure regions, for the deployment
object so you can use that as a means to supply this as an input parameter to the template. The SKU I don't think is available in code - this template has an example of showing the different SKUs available and how to set them as input values.
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've added a pricingTier parameter for the SKU.
Content/src/Server/Server.fsproj
Outdated
@@ -1,5 +1,5 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<Project Sdk="Microsoft.NET.Sdk.Web"> |
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.
What's the difference?
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.
With the Web version, you get a web.config file which is used by IIS / Azure App Service to start up the application through dotnet
and to pass through traffic from IIS to the app.
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.
So that would be applicable only to Azure
option? Should this be a conditional then?
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 thought this would've been conditional, yes. @theprash thoughts?
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.
@theimowski @isaacabraham, I thought it might be better to avoid a conditional branch on the project type just to reduce complexity a little bit if there's little or no downside in using the .Web
version for all versions of the template. Aside from the web.config file, it also adds some extra entries in Server.deps.json
which seem to not be very important, but I don't know what they're for.
I'm happy to make it conditional though...?
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.
OK I've done it 😀
Content/build.fsx
Outdated
client.UploadData(destinationUri, IO.File.ReadAllBytes zipFile) |> ignore | ||
) | ||
|
||
"Publish" ==> "DeployWebApp" |
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.
Can we put that into a single chain?
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.
Technically this is what the dependencies are (the ARM deploy only sets up Azure infrastructure) but @isaacabraham has already put them in a single chain and I'm happy with it.
Content/build.fsx
Outdated
@@ -124,6 +186,11 @@ Target "Docker" (fun _ -> | |||
==> "Docker" | |||
#endif | |||
|
|||
#if (Azure) | |||
"Clean" ==> "Publish" ==> "DeployWebApp" |
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.
as above
Started reviewing, but didn't yet manage to test myself |
Simplify FAKE chain. Improve error reporting for missing ARM arguments.
# Conflicts: # Content/build.fsx # Content/src/Server/ServerGiraffe.fs # Content/src/Server/ServerSaturn.fs # Content/src/Server/ServerSuave.fs
@@ -125,11 +138,15 @@ | |||
"condition": "(Fulma != \"login\")" | |||
}, | |||
{ | |||
"exclude": "**/Dockerfile", | |||
"exclude": "Dockerfile", | |||
"condition": "(!Docker)" |
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.
the condition should now change to "(Deploy != \"docker\")"
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.
Whoops I missed those. Fix now, thanks.
"condition": "(!Docker)" | ||
}, | ||
{ | ||
"exclude": "**/yarn.lock", | ||
"exclude": "arm-template.json", | ||
"condition": "(!Azure)" |
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.
similar to above
Content/build.fsx
Outdated
let armTemplate = @"arm-template.json" | ||
let resourceGroupName = "safe-" + environment | ||
let subscriptionId = try getBuildParam "subscriptionId" |> Guid.Parse with _ -> failwith "Invalid Subscription ID. This should be your Azure Subscription ID." | ||
let clientId = try getBuildParam "clientId" |> Guid.Parse with _ -> failwith "Invalid Client ID. This should be the Client ID of a Native application registered in Azure with permission to create resources in your subscription." |
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.
How do I generate clientId? Create a "Web App" in azure then what?
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 needs to be documented properly but you need to create an app in Active Directory, set it to "Native" and then give it permissions to Windows Azure. The application id is the client id you need to use.
Is the |
@theimowski what should we use instead? Environment variables usually work quite well across most systems I imagine. |
@theimowski, We needed to be able to configure the public folder path in order for the Azure deploy to work, at least with the method that we're using here. The server had to be at the root of the published folder so it could not refer to a "Client" directory outside of itself. I discussed with @isaacabraham a few different ways we could do this including adding a config file, which is not well supported yet by FAKE for .NET Core. We thought an environment variable would be the least intrusive method for now. It can be set easily for the app in Azure and other systems as Isaac mentioned above. |
Sorry I didn't make myself clear. I meant to ask if its required to have any variance for the |
Ok thanks, then again I'd it in for making that conditional just for azure option |
Yeah. We could've deployed both the "client" and "server" folders as now but that makes things somewhat more complicated in terms of build / packaging I think. Perhaps we'll revisit this in the future but for now I think what is here should work (apart from your comment about making the environment bit Azure-only - I think that we can do that.) |
@theimowski I've now created a (lengthy) guide on the docs site in the template-deployment branch. Can you give it a run through and see if it works / makes sense for you? We can then release this alongside that. |
I've now also added support for Suave, which is hosted in App Service slightly differently than Giraffe. |
I'll have a look on Tuesday |
…ite to the Trace output though.
@@ -150,7 +181,8 @@ | |||
} | |||
} | |||
] | |||
},"**/*.fsx": { | |||
}, | |||
"**/*.fs*": { |
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.
is this change intentional?
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 can see you also now use //#if
instead of #if
in .fsx
script - is that this change that allowed that? Maybe we can get rid of this entry completely then?
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.
@theprash one for you.
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 change was required to enable templating in the FSPROJ file. I believe the entry is still required for FSX too. The commenting of #if
was not related to this change.
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.
Right, makes sense. So you commented #if in fsx for better editor support? I think I like it this way now
I started testing the
Not the binding redirect for System.Net.Http at the beginning - not sure if that's desired? |
@theimowski The binding redirect with System.Net.Http is a red herring here I think. I've seen this issue this weekend with Microsoft.Rest.ClientRuntime.Azure. The "fix" is (and don't laugh) to make a change to the FAKE script e.g. add a new line anywhere in the script, and then re-run it. It then works. I have no idea why this is happening or how to prevent it from occurring in the first place. |
WTF, do we know why's that? If not the "fix" should be documented for sure |
@theimowski I have no idea. Can you test it - maybe it just happens on my machine! |
I'm also considering moving away from the SDK and shelling out to the |
Yes it worked :) Something FAKE-related? @matthid any ideas why resaving a fake script may have impact on assembly binding? This is using FAKE 4 |
So does this happen again after that and you need to make another change or does it always work now? |
Works now without need to make more changes. The |
No idea, so it is not reproduce-able at all? |
LGTM - I managed to deploy Saturn and Suave versions to azure. Can we add a note to docs about the FAKE modify-file "fix" until we know what's the real issue here? |
Or upgrade to latest version of fake ;) |
Yes that's probably a good idea anyway - added #70 |
This is now released in 0.15 |
Amazing work, thanks! :) |
@jarlestabell yeah. It's annoying - hopefully moving to FAKE 5 will "fix" this, but we don't know exactly why it's happening :-/ |
Adds ARM template and Azure deployment to FAKE script for #51.
I needed to introduce an environment variable to allow changing the public content path because the Azure deployment I was using required putting the server at the root.
This depends on #64.