Cloud 1524

[davejrt]

This PR will add the functionality to install the latest versions of docker-ce on all channels in the new docker repos. It maintains support for using the docker-engine package from the old repos based on the version number passed into the module.

It removed where necessary case statements for unsupported OS's like Archlinux, Gentoo and older versions of RHEL.

[scotty-c]

LGTM

[purplexa]

FYI, this was not a legitimate change to put in a patch release per SemVer. It should have required a major version bump since it changed the public API.

[scotty-c]

@thrnio I disagree with you. As it only added the new repo's which is actually abstracted from the user and changed a param name. No new functionality was added.

[purplexa]

8. Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API. It MAY include minor and patch level changes. Patch and minor version MUST be reset to 0 when major version is incremented.

The parameters on init.pp are the public API for a module like this, so changing a parameter name is a backwards incompatible change and requires a major version bump. A user using parameters as documented in the README for a module must always be able to upgrade patch releases without having to change the code that uses the module; otherwise, we break trust.

[LongLiveCHIEF]

He's totally right about this being a major version bump. It's why I was waiting to release an official 1.0 when I forked garethr-docker. I wanted all the param changes to be done before I made a release, since any param changes would require either a major version bump, or a graceful way to handle deprecation.

I really think though, that we're hurting maintainers in the first place by having all these static parameters to begin with, and then also having duplicate params for ce and ee. This is actually very anti-puppet.

You should be able to pass repo params as a hash, and we should use an asterisk to assign to either yumrepo or apt::source depending on the OS.

This allows users to change and add params as the underlying resource libraries change the modules they support, and it allows a far wider range of configuration while simultaneously reducing the parade of params and making documentation simpler.

For example, let's say i want to pull from upstream, but I want to do it through my own mirror. I don't want to have to disable package management and do all sorts of management of packages and and such on my own, and lose all the resource ordering that comes with it here.

class docker (
  $package_source_repository = {}
) {

  $repository_resource = $facts['os']['family'] ? {
    'RedHat' => 'yumrepo',
    'Debian' => 'apt::source'
  }

  # could set defaults the same way and assign to $docker_repository_defaults
  create_resources($repository_resource, $package_source_repository, $docker_repository_defaults)
} 

[davejrt]

Putting aside the issue of release versions, @LongLiveCHIEF as we've previously discussed, we're happy to take community PR's on this module.

I don't disagree that this part of the module could be simplified, however we also need to take into account that we want to make this easy to use for newcomers to docker.

The PR that was raised previously to refactor the module didn't support Puppet 4, or hiera 3 hence we couldn't accept it.

If this is something you want to contribute, then the team and myself will gladly look at a WIP PR and discuss options and opinions there. As you're working through the PR reach out to myself @davejrt or @scotty-c on the puppet community slack and we can discuss the changes so we're aligned