GH472: Added OctoPack tool #1218

cpx86 · 2016-09-10T17:50:23Z

See #472. Pretty much implemented in the exact same way as OctoPush.

dnfclas · 2016-09-10T17:50:27Z

Hi @cpx, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

trailmax · 2016-09-19T13:57:21Z

I'm trying your pull request in a custom build of Cake and run into an issue. In OctopusPackSettings property BasePath should really be DirectoryPath, not a FilePath. Unless I'm missing something.

Edit: same goes for OutFolder property.

gep13 · 2016-09-19T14:06:54Z

@trailmax said...
I'm trying your pull request in a custom build of Cake and run into an issue. In OctopusPackSettings property BasePath should really be DirectoryPath, not a FilePath. Unless I'm missing something.

@cpx is the same not also true for OutputFolder?

cpx86 · 2016-09-20T12:49:00Z

That does sound correct, I'll update the PR :)

cpx86 · 2016-09-20T13:06:53Z

@trailmax How did your cakefile look, and what version of Octo.exe do you have? For me it worked with Octo 3.4.2 and the following cake script:

Task("Pack")
        .Does(() => {
                OctoPack("MyPackage", new OctopusPackSettings {
                        Version = "1.0.0",
                        BasePath = "./bin",
                        OutFolder = "./out",
                        Overwrite = true
                });
        });

I'll change it anyway to DirectoryInfo since that is more correct - would just like to understand why there's a difference between our tests.

trailmax · 2016-09-20T14:13:41Z

@cpx Octo.exe - the latest. Yes, what you have it there worked - when you specify the path as a String it is fine. But I had a Parameters class with all the directories listed and my directories were recorded as DirectoryPath objects. And conversion from FilePath to DirectoryPath failed on compile time.

cpx86 · 2016-09-21T16:14:22Z

@trailmax: Got it, that explains it.

patriksvensson

First, I'm sorry for the delay in reviewing this PR.
I left some comments for you. Nothing major 😄

patriksvensson · 2016-09-27T23:35:12Z

src/Cake.Common/Tools/OctopusDeploy/OctopusPackFormat.cs

+        /// <summary>
+        /// NuGet package
+        /// </summary>
+        NuPkg,


I'm thinking if we should rename this to NuGet. What do you think?

NuPkg is the constant used by Octo.exe. From the help:
--format=VALUE Package format. Options are: NuPkg, Zip.

Yes, sounds good to me.

patriksvensson · 2016-09-27T23:35:27Z

src/Cake.Common/Tools/OctopusDeploy/OctopusPackFormat.cs

+namespace Cake.Common.Tools.OctopusDeploy
+{
+    /// <summary>
+    /// Represents the format of an Octopus package


Missing period after package.

patriksvensson · 2016-09-27T23:35:41Z

src/Cake.Common/Tools/OctopusDeploy/OctopusPackSettings.cs

+namespace Cake.Common.Tools.OctopusDeploy
+{
+    /// <summary>
+    /// Contains the settings used by OctoPack


Missing period after OctoPack.

patriksvensson · 2016-09-27T23:38:34Z

src/Cake.Common/Tools/OctopusDeploy/OctopusDeployPacker.cs

+                throw new ArgumentNullException(nameof(id));
+            }
+
+            var builder = new OctopusPackArgumentBuilder(id, _environment, settings);


Not that familiar with the Octopus tool wrappers, but is there any specific reason that you put the argument building into it's own class? I'm not against it but it does break the convention we've used so far (to my knowledge at least) of putting the argument building together with the tool. Like I said, not against it since I like the separation, just a thought.

I've done the same with my PR - I just followed the pattern in already existing functionality for Octopus. Also a lot of Octopus's parameters for different commands are the same - it helps to extract these out.

Ok, wasn't aware of the existing functionality. I guess this can be left as it is then. 😄

In this case though the argument builder isn't reusable for any other commands. Pack is a local command whereas all other commands work with a remote Octopus server. So it might make more sense to merge the OctopusPackArgumentBuilder into the tool.

I leave this decision to you. If the other Octo tools does it like this, we can follow the convention and refactor this later if it makes sense, but if you want to change it, it's also fine.

I'd prefer to merge it into the tool then, since these arguments currently are not reusable for any other Octo command.

patriksvensson · 2016-09-27T23:39:18Z

src/Cake.Common/Tools/OctopusDeploy/OctopusDeployPacker.cs

+        /// <returns>The name of the tool.</returns>
+        protected override string GetToolName()
+        {
+            return "Octo";


Perhaps this should be OctopusDeploy Pack or something similar?

What's the convention? Should the tool name match the associated Cake type/member names, or should it match the underlying tool (which is octo.exe in this case)?

This should be a human readable string just describing the tool in the log. For nuget.exe pack tool we have NuGet Pack if I remember correctly.

Actually, NuGet has the same tool name (NuGet) for all tools as well, coming from the NuGetTool base class.

patriksvensson · 2016-09-27T23:40:40Z

src/Cake.Common/Tools/OctopusDeploy/OctopusDeployAliases.cs

+        /// <param name="id">The package ID.</param>
+        /// <param name="settings">The settings</param>
+        [CakeMethodAlias]
+        public static void OctoPack(this ICakeContext context, string id, OctopusPackSettings settings = null)


Could we create an overload for this method (without the settings) instead of using default values. We've seen some funny things happening with older versions of Roslyn when doing this.

patriksvensson · 2016-09-27T23:40:47Z

src/Cake.Common/Tools/OctopusDeploy/OctopusDeployAliases.cs

+        }
+
+        /// <summary>
+        /// Packs the specified folder into an Octopus Deploy package


Missing period after package.

patriksvensson · 2016-09-27T23:46:27Z

src/Cake.Common.Tests/Unit/Tools/OctopusDeploy/OctoPackTests.cs

+            [Fact]
+            public void Should_Throw_If_Id_Is_Null()
+            {
+                var fixture = new OctopusDeployPackerFixture();


We normally divides the Arrange,Act,Assert in Given/When/Then comments.
Would this be possible to change?

public void Test() { // Given var fixture = new MagicFixture(); // When var result = fixture.DoMagic(); // Then Assert.Equal(42, result); }

Personally I prefer to skip the comments and just be strict in dividing the test using linebreaks :) But I'll change it so it matches the rest of the test suite

patriksvensson · 2016-09-28T00:07:57Z

src/Cake.Common/Tools/OctopusDeploy/OctopusPackArgumentBuilder.cs

+
+namespace Cake.Common.Tools.OctopusDeploy
+{
+    internal class OctopusPackArgumentBuilder


Class should be sealed.

patriksvensson · 2016-09-28T00:08:19Z

src/Cake.Common/Tools/OctopusDeploy/OctopusDeployPacker.cs

+    /// <summary>
+    /// The Octopus deploy package packer
+    /// </summary>
+    public class OctopusDeployPacker : Tool<OctopusPackSettings>


Class should be sealed.

cpx86 · 2016-09-28T07:10:16Z

I've pushed the requested changes now :) Naming of the format enum values and whether to have a separate argument builder is still open for discussion though.

trailmax · 2016-09-28T23:13:20Z

Regarding enum naming I prefer to keep it NuPkg because that's what the underlying tool is using. This way there is no divergence between Cake API and octo.exe API.

cpx86 · 2016-09-29T08:05:05Z

Merged the builder into the tool. I think the name should be kept as Octo since that seems to be the convention used by e.g. NuGet.

patriksvensson · 2016-10-16T10:14:25Z

@cpx Sorry for abandoning this PR. Life simply got in the way. I think this looks good for merging.

Could you rebase this PR against develop?

Thanks!

cpx86 · 2016-10-16T10:50:46Z

@patriksvensson NP, it happens :) will rebase as soon as I'm at my machine.

cpx86 · 2016-10-16T11:04:19Z

@patriksvensson Done!

patriksvensson · 2016-10-16T11:14:05Z

Merged! Thanks for this! 👍

dnfclas added the cla-already-signed label Sep 10, 2016

trailmax mentioned this pull request Sep 14, 2016

Option to deploy an existing release in OctopusDeploy #1228

Closed

cpx86 force-pushed the feature/GH-472 branch 2 times, most recently from 2002cb9 to d41005f Compare September 20, 2016 13:20

patriksvensson reviewed Sep 27, 2016

View reviewed changes

patriksvensson requested changes Sep 28, 2016

View reviewed changes

cpx86 force-pushed the feature/GH-472 branch from d41005f to 379a2d0 Compare September 28, 2016 07:08

cpx86 force-pushed the feature/GH-472 branch from 379a2d0 to 81683e8 Compare September 29, 2016 08:03

gep13 changed the title ~~Added OctoPack tool~~ GH472: Added OctoPack tool Oct 12, 2016

patriksvensson approved these changes Oct 16, 2016

View reviewed changes

patriksvensson added the Waiting for issuer feedback label Oct 16, 2016

Added OctoPack tool

089d546

cpx86 force-pushed the feature/GH-472 branch from 81683e8 to 089d546 Compare October 16, 2016 11:03

patriksvensson merged commit 089d546 into cake-build:develop Oct 16, 2016

patriksvensson mentioned this pull request Oct 16, 2016

Support OctoPack #472

Closed

gep13 removed the Waiting for issuer feedback label Oct 17, 2016

GH472: Added OctoPack tool #1218

GH472: Added OctoPack tool #1218

Conversation

cpx86 commented Sep 10, 2016

dnfclas commented Sep 10, 2016

trailmax commented Sep 19, 2016 • edited Loading

gep13 commented Sep 19, 2016

cpx86 commented Sep 20, 2016

cpx86 commented Sep 20, 2016

trailmax commented Sep 20, 2016

cpx86 commented Sep 21, 2016

patriksvensson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpx86 commented Sep 28, 2016

trailmax commented Sep 28, 2016

cpx86 commented Sep 29, 2016

patriksvensson commented Oct 16, 2016

cpx86 commented Oct 16, 2016

cpx86 commented Oct 16, 2016

patriksvensson commented Oct 16, 2016

trailmax commented Sep 19, 2016 •

edited

Loading