GH810: Support for octo push command, common Octopus base classes

[agc93]

Resolves #810

This PR integrates support for the octo.exe push command alongside the existing release creation. In order to play nice with the existing alias, I have created the OctopusDeployPusher as a separate Tool<T>.

Tests have all been added as well.

I have refactored the new Tool and Settings classes to use a new common OctopusDeployArgumentBuilder tied to the existing OctopusDeploySettings class. I have not however refactored the existing release creation alias to use this structure. It should be basically drop-in, but I will leave that as a call for someone more familiar with the code base.

This is a much bigger contribution than my usual, so feedback is welcome as always :)

[gep13]

@agc93 from a consistency stand point, this extra line should be removed here. Personally, I think we do need a linebreak here, but let's keep things the same for now.

[gep13]

@agc93 thank you very much for your continued contributions to Cake, and thanks for tackling this larger issue. I have left some comments in the code. Nothing major, just a few small things. If you can fix them up, adn then rebase on the most recent develop, I can take another look. Thanks!

[agc93]

Alright so I have made all the whitespace corrections and a couple of the refactorings (keeping it local since last time has made me scared of squash).

So the only outstanding point is the common arguments one above of moving server and apiKey out of the settings object (and potential future removal) in favour of alias parameters.

I must admit I'm not 100% on the rationale for that as it seems like the sort of thing that should be in a common class (as per existing design). Admittedly, I'm not as familiar with the existing conventions so happy to defer to you ( @gep13 ), or @devlead or @patriksvensson on this one.

[gep13]

@agc93 ha ha, I know exactly what you mean about squash/rebase. It was something that I was very much wary of before I got used to it. If you want, when it gets to that stage, we can maybe have a skype call, or something, and we can walk through it.

In terms of the rationale behind my comments for the parameters, it basically comes down to this. We always want things to "just" work. By that I mean, we want all aliases to be explicit about what parameters are required in order for it to function. In the case of this alias, the server and apikey are required, otherwise it will not function. While we could simply have these in the settings class, the user won't know from just looking at the alias definition, that those things have to be set. By making them direct input parameters, we are making it as clear as possible that they are required. All other things that "can" be set, are in the settings class.

There is the same thought process behind things like the MSBuild alias. The solution file path is an input parameter, rather than a property in the MSBuildSettings class.

So while i agree, we want to make sure that we re-use as much of the common settings class as possible, I don't want to do that whilst losing clarity on the intention of the alias. and how it operates.

Does that make sense?

[wwwlicious]

While I understand the rationale, this approach works fine until you need to change what is 'required' then you break the contract (method signature) and is IMO always painful. A single settings param is more flexible and will outlive the 'required' fields....always.

If docs and decent error messages are available when calling something without its required properties then it really isn't much of a hassle to call it a few times, figure out what's missing and add it.

Just my two cents.

[patriksvensson]

@wwwlicious Removing a parameter from the settings would also be breaking. Not removing the required setting (but obsoleting it) would not break it per se but would introduce a semantic change, which is even worse in my opinion. Showing required parameters in the alias method signature will give a clear picture to the consumer what is required and not.

[wwwlicious]

@patriksvensson I am not advocating making a breaking change (or obsoleting) existing api's here or elsewhere, my comment is simply to say that a 'required fields' pattern is more fragile to a consumer than a message based approach where you pass a single settings param to the method for the long term and therefore not one I would personally advocate.

Doc's, sensible defaults and guard clauses can easily identify what is required from what isn't. Contract stability is more important to me over the lifespan of a project when consuming 3rd party code than the short amount of time spent when initially wiring it up.

As I said, just my opinion so I'm not jumping up and down about it or anything! 🍰

[agc93]

@gep13 I will do a commit now of what I have and I might take you up on that offer once this is sorted 😄

[agc93]

Okay, so taking the "where should required fields" part aside, still leaves the challenge of the server address and API key arguments which now a) have to be separately implemented by each Octopus command implementation despite being common arguments and b) will now be in both the Settings class and the method call, meaning one has to be trusted over the other.

For a) this seems a little counter-intuitive to me since we are deliberately taking known common functionality (usually a base class in a nutshell) and moving it out of an existing base class, with all the deprecation problems that might introduce.. as per earlier, happy to make the change here, but that will mean that the create release still needs to be updated (with all that comes with that).

For b) I assume judging by the rationale so far, we would simply ignore any server address or API keys passed in the settings object since we think they will be removed in a future release. Does that sound correct?

[patriksvensson]

@agc93 Yes, it might seem counter constructove, but I see the greater benefit of a consistent API. This benefit outweights the implementation cost in my opinion.

[gep13]

@agc93 let me know when you are in a position to look at this, and to fix up the remaining issues, and we can have a chat about how to do the merge/squash/rebase.

[agc93]

@gep13 I had a go at being self-sufficient, how does that look? 😄

[gep13]

@agc93 after a very brief look, looks like the rebase/squashing has worked 👍 I will review it properly again, and I will get back to you if I have any comments.

[agc93]

Is there anything outstanding on this? Should I rebase again now or is it likely to take a while?

[gep13]

@agc93 I took the liberty of doing the rebase and merging into develop as there were a couple of small corrections that I thought were needed. I have merged it here: e864c22 As a result, I will close this PR, but the work has been included in the main develop branch, it is just that GitHub can't follow the flow. Thanks again!

[gep13]

@agc93 can I get you to raise an issue about removing the duplicated properties in the Settings files, that are now mandatory properties in the aliases? Thanks!