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

Refactor module #3

Merged
merged 3 commits into from
Oct 13, 2013
Merged

Refactor module #3

merged 3 commits into from
Oct 13, 2013

Conversation

jtopjian
Copy link
Contributor

@jtopjian jtopjian commented Oct 7, 2013

This commit refactors the original model. It removes the
apache-specific code from the init module and moves it to
a separate apache namespace. It also adds the ability to
configure apache either as a normal alias via conf.d or as
a vhost.

Note that the vhost has not been tested.

This commit refactors the original model. It removes the
apache-specific code from the init module and moves it to
a separate apache namespace. It also adds the ability to
configure apache either as a normal alias via conf.d or as
a vhost.

Note that the vhost has *not* been tested.
@jtopjian
Copy link
Contributor Author

jtopjian commented Oct 7, 2013

Not sure if you're interested in this or not. I know it can be awkward to have someone submit a refactor pull request like this, so I take no offence if you disregard this. :)

@nibalizer
Copy link
Member

This is pretty legit. I'm a bit busy at work right now but I'm open to merging this. Quick question:

Are you setting it to use the puppetlabs-apache class? Are we removing the module's ability to manage an apache vhost as a flat file?

Thanks a ton for the contribution.

@jtopjian
Copy link
Contributor Author

jtopjian commented Oct 7, 2013

Cool! No worries at all about this sitting for a while.

Yes - actually that reminds me to add puppetlabs-apache as a requirement in the Modulefile.

So this module itself won't manage the vhost entry -- it defers that responsibility to the puppetlabs-apache module. If the user simply wants an alias instead of a vhost, a standard template is still used. I would like to be able to use the apache module to do this for the same reasons as the vhost, but I don't think there's a way... I'll double check.

The reason I totally decoupled apache from the base module is to assist in configurations where someone wanted to use, say, nginx, or simply no web server at all.

I hope that answers your question?

@nibalizer
Copy link
Member

@jtopjian I don't see you adding the dependency on puppetlabs-apache. Did you make than change and forget to push? I feel really bad for letting this sit. I've just been crazy busy.


class { 'puppetboard': }
If you want puppetboard available through Apache, there are two manifests
to help:
Copy link
Member

Choose a reason for hiding this comment

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

We need some text here explaining what each does.

@jtopjian
Copy link
Contributor Author

@nibalizer Nope, I totally dropped the ball altogether on editing the Modulefile. I've also been very busy this week, so no worries at all. I'll try to get the Modulefile changed as well as add some more docs by the end of this weekend.

@nibalizer
Copy link
Member

Awesome. My big blocker is like you, time. I want to test your stuff before I just push it to the forge. I feel kinda terrible about the current forge release.

Added puppetlabs/apache as a module dependency. Added more detailed
documentation regarding the apache configuration.
@jtopjian
Copy link
Contributor Author

OK, just pushed the second commit.

@jtopjian
Copy link
Contributor Author

While playing around with the new pypuppetdb library, I just noticed that it's not included in this module. What's your opinion on whether it should be included or not?

I get too caught up in theoretical/academic arguments about these types of decisions. On one hand, pypuppetdb is a core library that puppetboard requires. On the other hand, if pypuppetdb is left for the user to include in their own compositional role manifest, it prevents a duplicate resource issue for the rare occurrence that a user was using pypuppetdb first and then decided to install puppetboard (this example is better argued at a larger scale :)

@jtopjian
Copy link
Contributor Author

OK, scratch that last comment. It's pulled in via the pip requirements file. Problem solved :)

Added more detailed documentation.
nibalizer added a commit that referenced this pull request Oct 13, 2013
@nibalizer nibalizer merged commit 360dbc4 into voxpupuli:master Oct 13, 2013
@nibalizer
Copy link
Member

Awesome Stuff. Thanks.

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

2 participants