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.

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

WIP: refactor package management #102

Closed
wants to merge 7 commits into from
Closed

WIP: refactor package management #102

wants to merge 7 commits into from

Conversation

LongLiveCHIEF
Copy link
Contributor

@LongLiveCHIEF LongLiveCHIEF commented Jan 6, 2018

This pull refactors package management to be more flexible for implementor's, and also to positiion the code/data better for puppet 5/6 support when it's time to bring this module up to speed.

Features and changes:

  • Removes the distinctions between ce and ee versions. Implementor needs to merely provide a package repository location.
  • Re-adds the docker_command parameter, making this module compatible with docker-latest redhat (and other) special versions of docker that use alternative naming schemes and binaries.
  • Uses dockerd when docker version is 17.06 or higher, and docker daemon when lower. (also takes into account docker binary name in case using docker-latest or other alternatively named binary)

Closes #45 #100 #38
Closes the following issues leftover from the garethr-docker fork:

@LongLiveCHIEF
Copy link
Contributor Author

@davejrt @scotty-c are you guys ok with how I'm using lookup here in the manifests/init.pp? It gives us the advantage of a "layer 3"-esque data source structure before the module is minimum support of puppet 5.

Back when I originally forked garethr-docker, I decided to support only puppet 5+, reasoning that anyone who still needed older could use the garethr-docker module, since there were likely running older infrastructure anyways.

If you are ok with this, it will make adding all of my patches back to this module much easier, since I opted for using data instead of if/else/case statements everywhere.

@LongLiveCHIEF
Copy link
Contributor Author

I think all I've got left to do here is documentation and some testing, and then I'll remove the WIP status of this request. There are a lot of parameter changes.

@scotty-c
Copy link
Contributor

scotty-c commented Jan 9, 2018

@LongLiveCHIEF we do need to support Puppet 4 with this module as it is supported and we have PE customers running versions that are Puppet Enterprise that use this module. That is why we have not used data in this module or the k8s module. One of the considerations on design we need to make is we have to please the many, which sometimes means we cant use the latest tools that would make it more efficient.

@LongLiveCHIEF
Copy link
Contributor Author

The stuff I've written is puppet 4 compatible is it not? Yes, I added data sources, but I use lookup instead of relying on APL. Isn't data-in-modules native to puppet 4?

@scotty-c
Copy link
Contributor

scotty-c commented Jan 9, 2018

We will have to test the backwards compatibility today to make sure there are no breaking changes. Are you also able to rebase this PR with master

@LongLiveCHIEF
Copy link
Contributor Author

We will have to test the backwards compatibility today to make sure there are no breaking changes.

👍

Are you also able to rebase this PR with master

👍

Yes, I just wanted to make sure you guys were ok with my approach before I wrapped this up and rebased. I also need to do that same said compatibility testing since I don't think the current tests alone will be enough to validate this one.

release: "${facts.os.distro.codename}"
repos: 'stable'
key:
source: "https://download.docker.com/linux/%{facts.os.name"
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a closing brace

@davejrt
Copy link
Contributor

davejrt commented Jan 11, 2018

@LongLiveCHIEF

I've left a few comments, but so far I've had to upgrade to puppet 5 just to make this work, and after fixing the things I've left comments I still end up with this error

Error: Could not find resource 'Package[docker]' in parameter 'before' on node node-01.corp.puppetlabs.net

You're welcome to grab me on the puppet slack if you want to have a chat either in the modules channel or directly. Otherwise happy to continue the conversation here. As @scotty-c said there are some concerns here about backwards compatibility but I think you've put forward some interesting ideas here that we can potentially use and incorporate.

@LongLiveCHIEF
Copy link
Contributor Author

@davejrt so after some digging, I realize that native data in modules was introduced in Puppet 4.3. According to the declared support of this module, that means I will have to remove the data sources entirely.

I know a large portion of your customer base is still on Puppet 4, but would you happen to know if it would be feasible to drop support for Puppet < 4.3? Just grasping at straws for one last chance to make the rest of my patches simple 😀

Either way, I'm going to close this one, refactor a bit, and re-open in a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants