Skip to content
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

Octopus deploy trace always marked failures #2428

Merged
merged 10 commits into from
Nov 29, 2019
Merged

Octopus deploy trace always marked failures #2428

merged 10 commits into from
Nov 29, 2019

Conversation

ids-pfinn
Copy link
Contributor

@ids-pfinn ids-pfinn commented Nov 7, 2019

Description

Two changes:

  1. A trace in the Octo.fs doesn't mark itself as successful before going out of scope so the output is always the Failed status

    • Prefix: image
    • Postfix: image
  2. Adds additional functions to return the exit code of the octopus command.

TODO

  • New (API-)documentation for new features exist (Note: API-docs are enough, additional docs are in help/markdown)
  • unit or integration test exists (or short reasoning why it doesn't make sense)
  • Change Process.execSimple to CreateProcess APIs - maybe.

matthid and others added 7 commits October 20, 2019 22:22
Release 5.18.0
* BUGFIX: Paket module was broken - #2413
* BUGFIX: New `CreateProcess.withToolType` wasn't working for `ToolType.FrameworkDependentDeployment` - #2412
* ENHANCEMENT: Add support for local dotnet tool to fake-template and make it the default.
* NEW: Add `Fake.DotNet.Testing.Coverlet`, thanks @Tarmil - #2413
* BUGFIX: `paket pack` module was broken, thanks @sergey-tihon - #2418
* BUGFIX: `Fake.DotNet.Testing.Coverlet` was not working, thanks @Tarmil - #2424
forgot how partial application works and that it is not just magic.
also forgot ignore is a function.
@ids-pfinn
Copy link
Contributor Author

ids-pfinn commented Nov 8, 2019

@matthid The FAKE-CI failed due to the TemplateIntegrationTests step.
I went through the build output and I think the tests are passing

Passed: �[36m 40
	Fake.Core.ProcessIntegrationTests/ProcessUtils.findLocalTool doesn't fail on non-existent path, #2390
	Fake.Core.ProcessIntegrationTests/Make sure cmd-echo works with regular redirect/lazy redirect
	Fake.Core.ProcessIntegrationTests/Make sure cmd-echo works with regular redirect/regular redirect
	Fake.Core.ProcessIntegrationTests/Make sure process with lots of output and error doesn't hang - #2401/lazy redirect
	Fake.Core.ProcessIntegrationTests/Make sure process with lots of output and error doesn't hang - #2401/regular redirect
	Fake.DotNet.MSBuild.IntegrationTests/#2112 (2)
	Fake.DotNet.MSBuild.IntegrationTests/#2112
	Fake.DotNet.MSBuild.IntegrationTests/#2134 (3)
	Fake.DotNet.MSBuild.IntegrationTests/#2134 (2)
	Fake.DotNet.MSBuild.IntegrationTests/#2134
	Fake.DotNet.NuGetIntegrationTests/getLastNuGetVersion works with myget - #2124
	Fake.DotNet.NuGetIntegrationTests/getLastNuGetVersion works - #2124
	Fake.DotNet.NuGetIntegrationTests/getLastNuGetVersion works - #2294
	Fake.DotNet.PaketIntegrationTests/findPaketExecutable works for global tools - #2361
	Fake.IO.FileIntegrationTests/Shell.mv works as expected when src is a dir dst is a dir - #2293
	Fake.IO.FileIntegrationTests/Shell.mv throws as expected when src is a dir and dst is a file - #2293
	Fake.IO.FileIntegrationTests/Shell.mv works as expected when src is a dir and dst doesnt exist - #2293
	Fake.IO.FileIntegrationTests/Shell.mv works as expected when src is a file dst is a dir - #2293
	Fake.IO.FileIntegrationTests/Shell.mv works as expected when src is a file and dst is a file - #2293
	Fake.IO.FileIntegrationTests/Shell.mv works as expected when src is a file and dst doesnt exist - #2293
	Fake.IO.FileIntegrationTests/Files created using File.create can be used immediately - #2183
	Fake.IO.FileIntegrationTests/File.tryGetVersion works when component is missing #2378
	Fake.IO.FileIntegrationTests/File.getVersion throws InvalidOperationException #2378
	Fake.Core.Globbing.IntegrationTests/glob should handle substring directories properly
	Fake.Core.Globbing.Tools.Tests/Test cannot find tool folder in sub path returns default
	Fake.Core.Globbing.Tools.Tests/Test find tool folder in sub path
	Fake.Core.Globbing.Tools.Tests/Test cannot find tool folder in sub path
	Fake.Core.Globbing.Tools.Tests/Test try find tool folder in sub path
	Fake.DotNet.CliIntegrationTests/Make sure dotnet installer works without spaces - #2319
	Fake.DotNet.CliIntegrationTests/Make sure dotnet installer can install into path with spaces - #2319
	Fake.DotNet.CliIntegrationTests/Make sure dotnet installer works in paths with spaces - #2319
	Fake.Core.IntegrationTests/issue #2007 - native libs work
	Fake.Core.IntegrationTests/issue #2025
	Fake.Core.IntegrationTests/reference fake core targets
	Fake.Core.IntegrationTests/use external paket.dependencies
	Fake.Core.IntegrationTests/context exists
	Fake.Core.IntegrationTests/reference fake runtime
	Fake.Core.IntegrationTests/simple runtime error
	Fake.Core.IntegrationTests/simple failed to compile
	Fake.Core.IntegrationTests/no dependencies hello world and casing #2314�[37m

I ran the TemplateIntegrationTests step locally and the tests passed.

@ids-pfinn
Copy link
Contributor Author

Nvm about the previous comment. Looks like they are passing now.

@ids-pfinn ids-pfinn changed the title [WIP] Octopus deploy trance always marked failures Octopus deploy trance always marked failures Nov 8, 2019
@matthid
Copy link
Member

matthid commented Nov 8, 2019

Maybe a flaky test, the error was:

25h[21:52:53 ERR] Fake.DotNet.Cli.IntegrationTests.Template tests/can install and run the template/fails to build a target that doesn't exist failed in 00:01:09.5680000. 
The script should recognize the target doesn't exist. Actual value was false but had expected it to be true.
   at Fake.DotNet.Cli.IntegrationTests.TemplateTests.tests@153-3.Invoke(Unit unitVar) in D:\a\1\s\src\test\Fake.DotNet.Cli.IntegrationTests\TemplateTests.fs:line 157
   at Expecto.Impl.execTestAsync@960-1.Invoke(Unit unitVar) in /Users/h/dev/haf/expecto/Expecto/Expecto.fs:line 964
   at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvoke[T,TResult](AsyncActivation`1 ctxt, TResult result1, FSharpFunc`2 part2) in E:\A\_work\130\s\src\fsharp\FSharp.Core\async.fs:line 398
   at <StartupCode$FSharp-Core>.$Async.StartChild@1650-5.Invoke(AsyncActivation`1 ctxt) in E:\A\_work\130\s\src\fsharp\FSharp.Core\async.fs:line 1650
   at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in E:\A\_work\130\s\src\fsharp\FSharp.Core\async.fs:line 109 <Expecto>

Failed:  1
	Fake.DotNet.Cli.IntegrationTests.Template tests/can install and run the template/fails to build a target that doesn't exist

Due to parallel build the output is further above. In any case I don't see this related to your changes. The Cli IntegrationTests are kind of flaky by design which is something I'm repeatedly think about but never come up with a good solution.

result

/// Creates a release and returns the exit code.
let createReleaseWithExitCode setParams =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is doing what you intended? From a quick look it looks like this either returns 0 or throws an exception (without actually testing it)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't tested that yet but looking at it again after the weekend, yes this seems wrong.

@ids-pfinn
Copy link
Contributor Author

ids-pfinn commented Nov 12, 2019

@matthid I added 2 tests but wanted to make sure me making the internals visible to the test project was an okay pattern for you before adding the rest.

Also I wasn't sure the best way to handle the exception being throwing because it has the commandString in the message. I ended up creating the commandline again but I feel like a better solution exists. ( This part https://github.com/fsharp/FAKE/pull/2428/files#diff-79e02955429be1b80880ec40c24d8c6fR319 )

@matthid
Copy link
Member

matthid commented Nov 29, 2019

I added 2 tests but wanted to make sure me making the internals visible to the test project was an okay pattern for you before adding the rest.

Yes it is. Thanks a lot! Looks very good now. Let's see how it works out!

@matthid matthid merged commit 5638727 into fsprojects:release/next Nov 29, 2019
@matthid matthid changed the title Octopus deploy trance always marked failures Octopus deploy trace always marked failures Dec 11, 2019
@matthid matthid mentioned this pull request Dec 14, 2019
@ids-pfinn ids-pfinn mentioned this pull request Feb 12, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants