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.

Sign up for GitHub

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

Switch from CliWrap to Shellfish #1046

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Switch from CliWrap to Shellfish #1046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4865431
Switch from CliWrap to Shellfish
borland Nov 28, 2024
bd9f1fb
TrimEnd; The process stdout has a trailing newline but the tests didn…
borland Nov 28, 2024
a144ea9
Update to latest shellfish
borland Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions source/Octopus.Tentacle.Tests.Integration/Octopus.Tentacle.Tests.Integration.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="CliWrap" Version="3.6.6" />
<PackageReference Include="Octopus.Shellfish" Version="0.2.2124" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.3.2" />
<PackageReference Include="NetCoreStack.DispatchProxyAsync" Version="2.2.0" />
<PackageReference Include="NSubstitute" Version="4.4.0" />
Expand All @@ -39,7 +39,7 @@
<PackageReference Include="Serilog" Version="2.12.0" />
</ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'net8.0'">
<PackageReference Include="System.DirectoryServices.AccountManagement" Version="8.0.0" />
<PackageReference Include="System.DirectoryServices.AccountManagement" Version="8.0.1" />
<PackageReference Include="System.Security.Permissions" Version="8.0.0" />
</ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'net48'">
Expand Down
14 changes: 6 additions & 8 deletions source/Octopus.Tentacle.Tests.Integration/Support/TentacleBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,11 @@
using System.Threading.Tasks;
using Autofac;
using Autofac.Util;
using CliWrap;
using CliWrap.Exceptions;
using Microsoft.Win32;
using Nito.AsyncEx;
using Nito.AsyncEx.Interop;
using NUnit.Framework;
using Octopus.Shellfish;
using Octopus.Tentacle.CommonTestUtils;
using Octopus.Tentacle.Configuration;
using Octopus.Tentacle.Tests.Integration.Util;
Expand Down Expand Up @@ -489,22 +488,21 @@ async Task RunCommandOutOfProcess(
ILogger logger,
CancellationToken cancellationToken)
{
async Task ProcessLogs(string s, CancellationToken ct)
void ProcessLogs(string s)
{
await Task.CompletedTask;
logger.Information($"[{commandName}] " + s);
commandOutput(s);
}

try
{
var commandResult = await RetryHelper.RetryAsync<CommandResult, CommandExecutionException>(
() => Cli.Wrap(targetFilePath)
var commandResult = await RetryHelper.RetryAsync<ShellCommandResult, Exception>(
() => new ShellCommand(targetFilePath)
.WithArguments(args)
.WithEnvironmentVariables(environmentVariables)
.WithWorkingDirectory(tmp.DirectoryPath)
.WithStandardOutputPipe(PipeTarget.ToDelegate(ProcessLogs))
.WithStandardErrorPipe(PipeTarget.ToDelegate(ProcessLogs))
.WithStdOutTarget(ProcessLogs)
.WithStdErrTarget(ProcessLogs)
.ExecuteAsync(cancellationToken));

if (cancellationToken.IsCancellationRequested) return;
Expand Down
21 changes: 10 additions & 11 deletions source/Octopus.Tentacle.Tests.Integration/TentacleCommandLineTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@
using System.Security.Principal;
using System.Text;
using System.Threading.Tasks;
using CliWrap;
using CliWrap.Exceptions;
using FluentAssertions;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
using NUnit.Framework;
using Octopus.Shellfish;
using Octopus.Tentacle.CommonTestUtils;
using Octopus.Tentacle.Tests.Integration.Support;
using Octopus.Tentacle.Tests.Integration.Support.TestAttributes;
Expand Down Expand Up @@ -119,7 +118,7 @@ public async Task VersionCommandTextFormat(TentacleConfigurationTestCase tc)

var expectedVersion = GetVersionInfo(tc);

stdout.Should().Be(expectedVersion.ProductVersion, "The version command should print the informational version as text");
stdout.TrimEnd().Should().Be(expectedVersion.ProductVersion, "The version command should print the informational version as text");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CliWrap deals with text output in a slightly different way to Shellfish.

If the app emits text without a trailing newline, CliWrap wouldn't give you a trailing newline either, whereas with Shellfish we get a trailing newline.

This isn't an inherent flaw in Shellfish, but rather just a side effect WithStdOutTarget(stringBuilder) output mechanism. The stringbuilder-writer puts a newline on the end of everything to keep things simple. We could do WithStdOutTarget(someCustomTarget) and get the output in a more direct way, but that would be harder and more complex than simply trimming the newlines in a few places

Copy link
Contributor

Choose a reason for hiding this comment

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

For the test I don't think it matters.

stderr.Should().BeNullOrEmpty();
}

Expand Down Expand Up @@ -267,7 +266,8 @@ public async Task CommandSpecificHelpAsJsonLooksSensibleToHumans(TentacleConfigu
""Description"": ""Show detailed help for this command""
}
]
}");
}
");
}

[Test]
Expand Down Expand Up @@ -359,7 +359,7 @@ public async Task ShowThumbprintCommandText(TentacleConfigurationTestCase tc)
"show-thumbprint", $"--instance={clientAndTentacle.RunningTentacle.InstanceName}");

exitCode.Should().Be(0, $"we expected the command to succeed.\r\nStdErr: '{stderr}'\r\nStdOut: '{stdout}'");
stdout.Should().Be(TestCertificates.TentaclePublicThumbprint, "the thumbprint should be written directly to stdout");
stdout.TrimEnd().Should().Be(TestCertificates.TentaclePublicThumbprint, "the thumbprint should be written directly to stdout");
stderr.Should().BeNullOrEmpty();
}

Expand All @@ -377,7 +377,7 @@ public async Task ShowThumbprintCommandJson(TentacleConfigurationTestCase tc)
"show-thumbprint", $"--instance={clientAndTentacle.RunningTentacle.InstanceName}", "--format=json");

exitCode.Should().Be(0, $"we expected the command to succeed.\r\nStdErr: '{stderr}'\r\nStdOut: '{stdout}'");
stdout.Should().Be(JsonConvert.SerializeObject(new { Thumbprint = TestCertificates.TentaclePublicThumbprint }), "the thumbprint should be written directly to stdout as JSON");
stdout.TrimEnd().Should().Be(JsonConvert.SerializeObject(new { Thumbprint = TestCertificates.TentaclePublicThumbprint }), "the thumbprint should be written directly to stdout as JSON");
stderr.Should().BeNullOrEmpty();
}

Expand Down Expand Up @@ -727,12 +727,11 @@ FileVersionInfo GetVersionInfo(TentacleConfigurationTestCase tentacleConfigurati
var output = new StringBuilder();
var errorOut = new StringBuilder();

var result = await RetryHelper.RetryAsync<CommandResult, CommandExecutionException>(
() => Cli.Wrap(tentacleExe)
var result = await RetryHelper.RetryAsync<ShellCommandResult, Exception>(
() => new ShellCommand(tentacleExe)
.WithArguments(arguments)
.WithValidation(CommandResultValidation.None)
.WithStandardOutputPipe(PipeTarget.ToStringBuilder(output))
.WithStandardErrorPipe(PipeTarget.ToStringBuilder(errorOut))
.WithStdOutTarget(output)
.WithStdErrTarget(errorOut)
.WithEnvironmentVariables(environmentVariablesToRunTentacleWith)
.ExecuteAsync(CancellationToken));

Expand Down
2 changes: 1 addition & 1 deletion source/Octopus.Tentacle.Tests/Octopus.Tentacle.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
<PackageReference Include="TeamCity.VSTest.TestAdapter" Version="1.0.36" />
</ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'net8.0'">
<PackageReference Include="System.DirectoryServices.AccountManagement" Version="8.0.0" />
<PackageReference Include="System.DirectoryServices.AccountManagement" Version="8.0.1" />
<PackageReference Include="System.Security.Permissions" Version="8.0.0" />
</ItemGroup>
<Target Name="ChangeAliasesOfStrongNameAssemblies" BeforeTargets="FindReferenceAssembliesForReferences;ResolveReferences">
Expand Down
1 change: 1 addition & 0 deletions source/Tentacle.sln.DotSettings
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpPlaceEmbeddedOnSameLineMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpRenamePlacementToArrangementMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpUseContinuousIndentInsideBracesMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002EMemberReordering_002EMigrations_002ECSharpFileLayoutPatternRemoveIsAttributeUpgrade/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EAddAccessorOwnerDeclarationBracesMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EAlwaysTreatStructAsNotReorderableMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002ECSharpPlaceAttributeOnSameLineMigration/@EntryIndexedValue">True</s:Boolean>
Expand Down