Add --pull and --no-cache options to Build/Publish Docker Image-Step

[ybroeker]

This pull request add's the --no-cache and --pull options requested in #612

[pjdarton]

Thanks for the PR - looks like this is taking things in the right direction 👍

While I wholeheartedly approve of the addition of this capability, I do have some concerns - as-written, these code changes alter the publicly-visible API to the plugin, which could break existing pipeline code for some users, which would be a nasty shock for any existing users calling this build step (or action) from their builds.
Breaking people's builds, even for good reasons, is a Bad Thing, so we need to keep the publicly-visible method signatures (anything public that isn't marked as @Restricted(NoExternalUse.class), which is a lot of code in here) as-is ... although we can deprecate existing ones and/or add new ones.

The recommended method of adding new parameters is to have @DataBoundSetter setters that set the instance's fields. In fact,, ideally, we'd have an (nearly) empty constructor and lots of set methods (constructors should only take mandatory fields and there shouldn't be many of those) ... and if we're going to mess with constructors then we should daisy-chain them together instead of having repeated code.

So, what we need is:

1. Replace the final keyword on each non-mandatory field with the comment /* almost final */
2. Add a set method annotated with @DataBoundSetter for every field that isn't final anymore.
3. Add a constructor that only takes non-mandatory fields as arguments (that might well be "no arguments"!).
4. Deprecate all existing constructors. Please add a Javadoc comment telling folks which constructor they should be using and to use the set methods.
5. Chain existing constructors so they call the new mandatory-arg-only constructor
6. Constructors that set non-mandatory fields should call the setter rather than setting the field directly.
7. Feel free to add a constructor that takes all fields (and sets all of them) that's annotated as @Restricted(NoExternalUse.class) with a Javadoc comment saying it's for internal use only, and use that anytime code inside this plugin needs to construct a new instance (this way, if someone adds a new field, they'll see all the other places in the code that might be affected).

If any of what I've written isn't 100% clear I'd suggest you do this to the build step class (DockerBuilderPublisher) first and, once we're both happy with how that looks, then you can apply the same kind of changes to the other one.
If you prefer to do so, you could do the refactor in a separate PR and then re-base this one on that, or you could do it all inside this one; I don't mind which.

TL;DR: These two classes need to be refactored so that extending their functionality won't break people's builds before they can be safely extended.

[ybroeker]

Thanks for your Feedback!

I reverted the constructors back to their old signature, and added setters for the new fields. Since only DockerBuilderPublisher used @DataBoundConstructor, I also only added @DataBoundSetter there. Is it enough or should I also add it to the setters in DockerBuildImageAction?

[pjdarton]

Looking much better - some (minor) cosmetic issues remaining.

[pjdarton]

My understanding is that it's only necessary to add @DataBoundSetter when extending a @DataBoundConstructor so, as there's no data-bound-anything in DockerBuildImageAction there isn't any need for the annotation on its setters.
i.e. the approach you've taken is correct.

[pjdarton]

Code changes went into released of docker-plugin version 1.1.9