-
Notifications
You must be signed in to change notification settings - Fork 585
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
Add support for Kudu Zip Deploy #1757
Add support for Kudu Zip Deploy #1757
Conversation
@isaacabraham, you may find this interesting. I started using it for our deployments, and it works quite well. Any modifications or suggested improvements are welcome. |
src/app/FakeLib/AzureKudu.fs
Outdated
Net.HttpWebRequest.Create(uri.AbsoluteUri + "api/zipdeploy", | ||
Method = "POST", | ||
ContentType = "multipart/form-data", | ||
Timeout = 300000) :?> Net.HttpWebRequest |
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.
We might want to make this a parameter. If so, should this be a TimeSpan
, time in seconds, or is milliseconds okay?
src/app/FakeLib/AzureKudu.fs
Outdated
if response.StatusCode = Net.HttpStatusCode.OK then | ||
logfn "Deployed %s" uri.AbsoluteUri | ||
else | ||
failwithf "Failed to deploy package with status code %A" response.StatusCode |
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 correct way to fail? What additional information should I provide?
src/app/FakeLib/AzureKudu.fs
Outdated
// Get the response. If 200 OK, then the deploy succeeded. Otherwise, the deploy failed. | ||
use response = request.GetResponse() :?> Net.HttpWebResponse | ||
if response.StatusCode = Net.HttpStatusCode.OK then | ||
logfn "Deployed %s" uri.AbsoluteUri |
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 it appropriate to log out success? It's possible to request an asynchronous deployment that can be tracked by calling another API to check status. I used this approach for its simplicity and because I don't want to poll for status updates. However, I could understand a desire to switch to the polling model, if that's desired.
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.
Please be aware that synchronous zip-deploys return 200 OK even if the deploy fails. I've suggested that they give better feedback on push deploys, but it doesn't look like they are prioritizing it: projectkudu/kudu#2641
You should probably do another call on 200 OK to check if the deploy actually succeeded or not.
src/app/FakeLib/AzureKudu.fs
Outdated
// Write the zip file to the request stream, then flush and close it to send. | ||
do use fileStream = new FileStream(zipFile, FileMode.Open) | ||
use inFile = request.GetRequestStream() | ||
fileStream.CopyTo(inFile) |
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 seemed better than reading all the bytes and writing them. I also went with the synchronous (versus asynchronous) APIs. What's preferred?
@matthid what does the |
In order to get rid of the old code-base I'd like to have code migrated to fake 5 modules. See http://fake.build/contributing.html#General-considerations |
@matthid I'm happy to move the module to a Fake 5 module. It would be helpful to point out an example in the |
@panesofglass you mean in addition to the steps outlined in http://fake.build/contributing.html#Porting-a-module-to-FAKE-5? Maybe a link to a PR like #1702 is sufficient? If yes please feel free to add that with this PR. |
Sorry, I've not been able to get back to this. Feel free to leave it open if you want. I may be able to work on it some more during the MVP Summit. Otherwise, feel free to close it. |
3016c5f
to
a94b9b1
Compare
a94b9b1
to
01c4bb7
Compare
90cc7d5
to
e001a5b
Compare
@matthid I'm not sure why the travis build is failing. What are the |
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 just marked the place I think relevant for travis... will review again before merging, but it looks pretty good. nice!
Fake.sln
Outdated
@@ -4,77 +4,85 @@ VisualStudioVersion = 15.0.27130.2026 | |||
MinimumVisualStudioVersion = 15.0.26124.0 | |||
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "app", "app", "{7BFFAE76-DEE9-417A-A79B-6A6644C4553A}" | |||
EndProject | |||
Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "Fake.Core.Context", "src/app/Fake.Core.Context/Fake.Core.Context.fsproj", "{D3D92ED7-C2B9-46D5-B611-A2CF0C30C8DB}" | |||
Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "Fake.Core.Context", "src\app\Fake.Core.Context\Fake.Core.Context.fsproj", "{D3D92ED7-C2B9-46D5-B611-A2CF0C30C8DB}" |
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 think dotnet core on linux has problems with this change. I think if you just replace \ with / and I think it will work.
Don’t ask me, feel free to report a bug to the dotnet guys...
@matthid Thanks! I changed the |
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 ok with the changes (most comments are more of kind "suggestion" :)
The only real question is if we need documentation for these? (I'm thinking of something basic like https://fake.build/api-slack.html)
open Fake.IO.Globbing.Operators | ||
|
||
/// Configuration details for packaging cloud services. | ||
[<CLIMutable>] |
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 remove CLIMutable
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.
I copied all this from the FakeLib
project in an attempt to be helpful to get this moved to Fake 5.
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.
Yes I know and this helps a lot! I just like APIs to be more consistent in fake5 so we can improve the code while porting - this moves the project forward. I see that people want to use stuff on fake5. That’s why I said the items are suggestions just in case you agree with them and have a minute ;)
/// The name of the role in the service. | ||
WorkerRole : string | ||
/// The SDK version to use e.g. 2.2. If None, the latest available version is used. | ||
SdkVersion : float option |
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.
Version is a float?
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 copied all this from the FakeLib
project in an attempt to be helpful to get this moved to Fake 5.
|> Option.map(fun path -> Path.Combine(path, (packageCloudServiceParams.CloudService + ".cspkg"))) | ||
|> Option.map(sprintf "/out:%s") | ||
|> defaultArg | ||
<| "" |
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 is a bit unusual
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 copied all this from the FakeLib
project in an attempt to be helpful to get this moved to Fake 5.
FileName = AzureEmulatorDefaults.StorageEmulatorToolPath.Value | ||
Arguments = "stop" }) AzureEmulatorDefaults.TimeOut with | ||
| Ok | StorageAlreadyStopped -> () | ||
| _ -> failwithf "Azure Emulator Failure on stop Storage Emulator" |
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.
Maybe we need some more information here, if you want to hide it you can use inner exceptions, like
raise <| exn("Azure Emulator Failure ...", exn(sprintf "Process exited with code %d" exitCode))
But that is only a suggestion if you left that information out on purpose, otherwise we might just include the exit code in the error message...
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 copied all this from the FakeLib
project in an attempt to be helpful to get this moved to Fake 5.
open System | ||
open System.IO | ||
#if NETSTANDARD | ||
open System.Net.Http |
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 always use System.Net.Http
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.
Is it available in full framework? I copied this from the Fake.Api.Slack
project.
src/app/Fake.Azure.Kudu/Kudu.fs
Outdated
/// Stages a set of files into a WebJob folder in the temp deployment area, ready for deployment into the website as a webjob. | ||
let stageWebJob webJobType webJobName files = | ||
let webJobPath = getWebJobPath webJobType webJobName | ||
Directory.CreateDirectory webJobPath |> ignore |
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.
Maybe Directory.ensure
or however it is called ;)
src/app/Fake.Azure.Kudu/Kudu.fs
Outdated
#endif | ||
open Fake.Core | ||
open Fake.Core.Environment | ||
open Fake.Core.Trace |
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'd recommend using Trace.*
instead of opening. We might add RequireQualifiedAccess
there as well...
Plan is to release this with the next release in a couple of days ;) |
@matthid what else would you like me to do? I am not very confident in changing things in Emulators and CloudServices having not used nor reviewed them. I'm happy to use only System.Net.Http if that's available in both the Core and full framework versions. Anything else? |
Like I said all is optional, this pr is green and I will merge and release it as is the next time I’m able to :) (sorry I’m clarifying because my communication is not always the best) Regarding system.net.http I’d need to look at the nuget package, but it might be that it doesn’t support the framework we need. I can take a look after merging and try to fix here and in the slack package... |
Thanks! |
Migrates all
Fake.Azure
modules to FAKE 5 and adds support for Kudu Zip Deploy. The initial implementation defines aZipDeployParams
type but does not define defaults.FAKE 4 additions:
FAKE 5 Modules: