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

Remove broken configtest_enable option #921

Merged
merged 1 commit into from
Oct 12, 2016
Merged

Remove broken configtest_enable option #921

merged 1 commit into from
Oct 12, 2016

Conversation

wyardley
Copy link
Collaborator

@wyardley wyardley commented Oct 11, 2016

Per #916, the $configtest_enable option doesn't seem if it could possibly work. In general, it would be good to review if everything here does what it is supposed to, but I think this will do the right thing on most platforms without any additional work.

I'm not sure if anyone would be using the $service_restart command directly, I'd think in most cases, we should be relying on $service_name and letting Puppet do the right thing, though I'm open to suggestion if people think $service_restart still needs to be exposed directly. What I don't understand is that, though it defaults to /etc/init.d/nginx reload, which doesn't work, the systems I tested all were using systemctl to restart the service anyway.

We still may want to revisit some kind of reporting back if the config test fails in a more generic way. I'm not going to address that one way or another for now.

@wyardley wyardley changed the title Remove broken and unhelpful configtest_enable option Remove broken configtest_enable option Oct 11, 2016
@wyardley
Copy link
Collaborator Author

wyardley commented Oct 11, 2016

passed acceptance tests for
centos-66-x64
centos-72-x64
ubuntu-server-1404-x64
debian-78-x64

Getting a (presumably unrelated) error running on debian-82-x64

/usr/local/lib/ruby/gems/2.3.0/gems/beaker-2.51.0/lib/beaker/host.rb:351:in `exec': Host 'debian-82-x64' exited with 1 running: (Beaker::Host::CommandFailure)
 puppet agent --configprint hiera_config
Last 10 lines of output were:
        from /usr/lib/ruby/vendor_ruby/puppet/util.rb:14:in `<top (required)>'
        from /usr/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /usr/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /usr/lib/ruby/vendor_ruby/puppet.rb:8:in `<top (required)>'
        from /usr/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /usr/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /usr/lib/ruby/vendor_ruby/puppet/util/command_line.rb:12:in `<top (required)>'
        from /usr/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /usr/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /usr/bin/puppet:7:in `<main>'
    from /usr/local/lib/ruby/gems/2.3.0/gems/beaker-2.51.0/lib/beaker/host.rb:39:in `[]'
    from /usr/local/lib/ruby/gems/2.3.0/gems/beaker-2.51.0/lib/beaker/dsl/install_utils/foss_utils.rb:300:in `block in install_puppet_on'

…t of the restart on many platforms anyway).
@bastelfreak bastelfreak merged commit 6792080 into voxpupuli:master Oct 12, 2016
@tux-o-matic
Copy link

@wyardley you removed a feature by deleting 'service_restart'.
One example, Docker, when running Nginx within a container rather than a initd/systemd service, you can use 'service_restart' to have the Puppet module interact with the container.

@wyardley
Copy link
Collaborator Author

@tux-o-matic: I can re-implement $service_restart separately (though I'd argue that it's generally a Good Thing to just use the $service_name parameter most of the time, and let puppet handle the service itself).

In my earlier test, even with $configtest_enable set, I couldn't get Puppet to call the specified custom init script, however, I am able to get that to work now. It is easy enough to restore this functionality without $configtest_enable; I'll open another PR for that (#927)

@tux-o-matic
Copy link

Thanks @wyardley. For Nginx users wishing to mix Puppet and Docker, you can't just rely on the Puppet Service type (until we get a Docker provider for Service).

darken99 pushed a commit to darken99/puppet-nginx that referenced this pull request Oct 17, 2016
…r (see comments in voxpupuli#921). Pass on $nginx::service_foo directly to service class
@wyardley wyardley deleted the fix_service_calls_issue_916 branch October 18, 2016 04:25
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
…e_916

Remove broken configtest_enable option
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
…r (see comments in voxpupuli#921). Pass on $nginx::service_foo directly to service class
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
…e_916

Remove broken configtest_enable option
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
…r (see comments in voxpupuli#921). Pass on $nginx::service_foo directly to service class
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