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

Scottx611x/docker puppet install #1664

Merged
merged 11 commits into from Apr 5, 2017
Merged

Conversation

scottx611x
Copy link
Member

No description provided.

@scottx611x scottx611x requested review from hackdna and drj11 April 4, 2017 14:36
@codecov-io
Copy link

codecov-io commented Apr 4, 2017

Codecov Report

Merging #1664 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1664   +/-   ##
========================================
  Coverage    37.69%   37.69%           
========================================
  Files          369      369           
  Lines        24341    24341           
  Branches      1260     1260           
========================================
  Hits          9175     9175           
  Misses       15166    15166

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2660dca...645365b. Read the comment docs.

@@ -1,5 +1,6 @@
forge "https://forgeapi.puppetlabs.com"

mod 'garethr-docker', '5.3.0'
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the puppetlabs/docker_platform?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, it looks exactly the same as: garethr-docker
Actually it depends on it: https://forge.puppet.com/puppetlabs/docker_platform/dependencies

Copy link
Member

Choose a reason for hiding this comment

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

The benefit of the puppetlabs modules is that they are provided by the Puppet team. Also, garethr-docker was last updated about a year ago and it supports only up to Ubuntu 14.04. The puppetlabs version was last updated a week ago and supports Ubuntu 16.04. Basically, it looks like it is probably going to replace garethr-docker.

@@ -0,0 +1,5 @@
class refinery::docker_ {
Copy link
Member

Choose a reason for hiding this comment

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

Why not refinery::docker?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because there is already a classdocker and Puppet complains about a naming conflict

@@ -229,6 +226,7 @@
Class["ui"],
Class["solr"],
Class["neo4j"],
Class["docker"],
Copy link
Member

Choose a reason for hiding this comment

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

Why does supervisor require docker?

Copy link
Member Author

@scottx611x scottx611x Apr 4, 2017

Choose a reason for hiding this comment

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

I don't think supervisor explicitly needs docker, but i don't think we want to start up everything without it since our app will depend on docker working.

Copy link
Member

Choose a reason for hiding this comment

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

I'd remove that since there is no explicit dependency and docker installation will still happen by the time the Refinery manifest is applied.

@@ -1,6 +1,6 @@
class refinery::selenium {
$geckodriver_version = 'v0.15.0'
$filename = "geckodriver-${geckodriver_version}-linux32.tar.gz"
$filename = "geckodriver-${geckodriver_version}-linux64.tar.gz"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because Geckodriver is not providing 32 bit builds in its newer releases anymore

@@ -0,0 +1,5 @@
class refinery::docker_ {
class { 'docker':
Copy link
Member

Choose a reason for hiding this comment

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

This should be class { '::docker': to indicate global namespace explicitly

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, should this be changed throughout our manifests? I was just following the syntax that we had used throughout

Copy link
Member Author

Choose a reason for hiding this comment

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

Also this answer is from 2012, is it still relevant?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's the reason you got the naming error when trying to use refinery::docker on the line above. Here's the source in Puppet docs (still relevant as of Puppet v3.5 and we are using v3.4.3): https://docs.puppet.com/puppet/3.5/lang_namespaces.html#workaround

Copy link
Member

@hackdna hackdna Apr 4, 2017

Choose a reason for hiding this comment

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

And yes, this should be changed in the rest of the manifests but in a separate branch.

@scottx611x scottx611x requested a review from hackdna April 5, 2017 15:44
@@ -0,0 +1,5 @@
class refinery::docker {
class { '::docker':
docker_users => [$app_user]
Copy link
Member

Choose a reason for hiding this comment

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

$::app_user

@scottx611x scottx611x removed the request for review from drj11 April 5, 2017 18:35
@scottx611x scottx611x merged commit 2ce7988 into develop Apr 5, 2017
@scottx611x scottx611x deleted the scottx611x/docker_puppet_install branch April 5, 2017 18:35
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.

None yet

3 participants