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

Add missing stream dirs and create streams from hiera #879

Merged
merged 1 commit into from
Oct 10, 2016
Merged

Add missing stream dirs and create streams from hiera #879

merged 1 commit into from
Oct 10, 2016

Conversation

andybotting
Copy link
Contributor

This creates the missing streams-available and streams-enabled
directories if stream is true and allow the streamhost resources
to be created from hiera.

Some simple tests are included.

This commit might replace PRs:

This creates the missing streams-available and streams-enabled
directories if stream is true and allow the streamhost resources
to be created from hiera.

Some simple tests are included.
@bastelfreak
Copy link
Member

Thanks for the PR @andybotting!

@bastelfreak bastelfreak merged commit d48b434 into voxpupuli:master Oct 10, 2016
@wyardley
Copy link
Collaborator

@andybotting: This affects #878 somewhat. I've rebased against your PR, but would love it if you can take a look and see if my changes seem sane (and if you have any suggestions to make the code cleaner or less spaghetti like).

I'm adding some additional tests related to this in there as well.

@wyardley
Copy link
Collaborator

wyardley commented Oct 10, 2016

What's the point of only creating certain dirs if $stream = true, when it will still try to create the resource when defined if $stream = false? Maybe the parameter needs to be changed, or just deprecate get rid of the $stream parameter entirely? @bastelfreak @andybotting

When creating the streams-available dir is wrapped in an if $stream block
https://github.com/voxpupuli/puppet-nginx/blob/master/spec/classes/nginx_spec.rb#L36

I get:

  1) nginx with defaults should compile into a catalogue without dependency cycles
     Failure/Error: it { is_expected.to compile.with_all_deps }
       error during compilation: Could not retrieve dependency 'File[/etc/nginx/streams-available]' of Concat[/etc/nginx/streams-available/streamhost1.conf]
     # ./spec/classes/nginx_spec.rb:22:in `block (3 levels) in <top (required)>'

after adding a 'require' on that file (something I added in #878, modeling what we did in #906).

I'm kind of of a mind to just get rid of / deprecate the if $stream bits, or, if we really want this logic, maybe rename it to make it clear that it's just ensuring that directory that the parameter is doing?

@andybotting
Copy link
Contributor Author

@wyardley I was aiming for the least amount of changes to get the functionality I needed, so I just kept that conditional for the directory. Personally, I'm not a big fan of the Debian style *-available *-enabled directory thing, so I like your changes in #878.

I wouldn't change this now that its merged, but I would also prefer to just always create the stream directories in this case and forget about the $stream conditional.

@wyardley
Copy link
Collaborator

I wouldn't change this now that its merged, but I would also prefer to just always create the stream directories in this case and forget about the $stream conditional.

Yes, in the latest iteration of #878, I ended up just dropping $stream entirely, since it wasn't really doing much except controlling whether those dirs were ensured (and we're already ensuring a lot of other dirs anyway, so I don't see that behavior as being particularly harmful).

I don't use this feature much, so would be very appreciative if you could give the stream-related changes in #878 at some point (especially the spec tests I added) to make sure things look reasonable.

cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
Add missing stream dirs and create streams from hiera
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
Add missing stream dirs and create streams from hiera
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
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