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

Warning and refresh even with no configs in the class declaration #905

Closed
ubellavance opened this issue Oct 8, 2016 · 18 comments
Closed

Comments

@ubellavance
Copy link

Hi,

I have absolutely nothing in my class, but I'm still getting hte warning to put everything in Hiera (which I do).

My class declaration:

class profile::rpnginx  {

  class { 'nginx':  }

}

The warning. As you can see, the URL should be updated to voxpopuli

Notice: [nginx] *** DEPRECATION WARNING***: HI! I notice that you're declaring some attributes in Class[nginx]. It is highly recommended to set these values via Hiera going forward. This will become mandatory in the near future. Please check out https://github.com/jfryman/puppet-nginx/blob/master/docs/hiera.md for more information.
Notice: /Stage[main]/Nginx::Notice::Config/Notify[[nginx] *** DEPRECATION WARNING***: HI! I notice that you're declaring some attributes in Class[nginx]. It is highly recommended to set these values via Hiera going forward. This will become mandatory in the near future. Please check out https://github.com/jfryman/puppet-nginx/blob/master/docs/hiera.md for more information.]/message: defined 'message' as '[nginx] *** DEPRECATION WARNING***: HI! I notice that you're declaring some attributes in Class[nginx]. It is highly recommended to set these values via Hiera going forward. This will become mandatory in the near future. Please check out https://github.com/jfryman/puppet-nginx/blob/master/docs/hiera.md for more information.'
Notice: Finished catalog run in 1.83 seconds

Thanks,

@wyardley
Copy link
Collaborator

wyardley commented Oct 8, 2016

In that simple example, do you have nginx::config defined in hiera? If not, try using something like the (clunky-looking, but working) example at:
https://github.com/voxpupuli/puppet-nginx/blob/master/docs/hiera.md

I think the problem is that if you don't declare nginx::config at all, the module still sets some default values for it. If you are setting some config variables in hiera, it is odd that you're seeing that warning.

As far as the outdated URL, I'll update it.

@ubellavance
Copy link
Author

Yes I do have many nginx::config in hiera:

# grep nginx::config /etc/puppet/hiera/host/server.example.com.yaml | grep -v ^#
nginx::config::http_cfg_append:
nginx::config::conf_dir: '/etc/opt/rh/rh-nginx18/nginx'
nginx::config::client_body_temp_path: '/var/opt/rh/rh-nginx18/lib/nginx/tmp/client_body'
nginx::config::proxy_temp_path: '/var/opt/rh/rh-nginx18/lib/nginx/tmp/proxy'
nginx::config::vhost_purge: true
nginx::config::confd_purge: true
nginx::config::log_dir: '/var/opt/rh/rh-nginx18/log/nginx'
nginx::config::gzip_types: 'text/plain text/css application/json application/x-javascript text/xml application/xml application/xml+rss text/javascript'

Unless you meant just nginx::config. If that is the case, I don't know exactly what you'd mean.

@wyardley
Copy link
Collaborator

wyardley commented Oct 9, 2016

Just out of curiosity, if you do include nginx instead of class { nginx: }, do you still get the warning?

Based on the link above, I think you need to use that clunkier example (with the anchor block) to define it that way.

@ubellavance
Copy link
Author

Still getting the warning using include nginx.

@wyardley
Copy link
Collaborator

wyardley commented Oct 9, 2016

Are you setting nginx::foo in hiera?

If you're setting any of the parameters I mentioned in #895 in nginx::XX, that's probably why you're getting that deprecation warning. Setting them in hiera is more or less equivalent to declaring the class with them.

I've tried a pretty vanilla setup both using Hiera and not, both with Puppet 3 and 4, and haven't seen this warning.

@ubellavance
Copy link
Author

Yes, I set many nginx::foo in Hiera, since I'm using the nginx package from Red Hat Sofware Collections, which uses unusual paths (and I also have non-standard requirements):

nginx::worker_processes: 'auto'
nginx::manage_repo: false
nginx::package_name: 'rh-nginx18-nginx'
nginx::pid: '/var/opt/rh/rh-nginx18/run/nginx/nginx.pid'
nginx::http_access_log: '/var/opt/rh/rh-nginx18/log/nginx/access.log'
nginx::nginx_error_log: '/var/opt/rh/rh-nginx18/log/nginx/error.log'
nginx::service_name: 'rh-nginx18-nginx'
nginx::service::service_restart: '/opt/rh/rh-nginx18/root/usr/sbin/nginx -t && systemctl reload rh-nginx18-nginx.service'
nginx::server_tokens: 'off'
nginx::log_format:
nginx::proxy_connect_timeout: '5400'
nginx::proxy_read_timeout:    '5400'
nginx::proxy_send_timeout:    '5400'
nginx::proxy_hide_header:
nginx::proxy_set_header:
nginx::nginx_vhosts:
nginx::nginx_locations:

@wyardley
Copy link
Collaborator

wyardley commented Oct 9, 2016

If you're declaring nginx::foo in hiera, you don't have "absolutely nothing in my class"; you're just declaring it in hiera instead of in the manifest.

I showed you the sections of code with the parameters for nginx::config; anything in there that's currently in nginx:: should be moved. Basically, anything that's configuring nginx (vs. configuring how the module configures it or manages the service) is in nginx::config currently.

If you want to think of it differently, move everything to nginx::config and then take any parameters in these lines (or just change them all and see which ones fail):
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/init.pp#L105-L130
back to nginx::.

Yes, the docs are terrible, but with the information I gave you earlier, you should be able to update your hiera configs as needed.

@ubellavance
Copy link
Author

Ok, so it is normal that the current code is not consistent with the philosophy. Work in progress I guess. I can understand, no problem then. I don't have enough knowledge to contribute for that so I'll concentrate on other areas. Thanks,

@wyardley
Copy link
Collaborator

wyardley commented Oct 9, 2016

Not sure I follow?

I think the existing code, while hard to read, is more or less consistent with the direction the previous maintainer was taking. While you can use some of the configuration directives at the top level, you will get a deprecation warning if you do it.

The nginx::nginx_vhosts and nginx::locations ones are probably an exception because they're so commonly used, but all the general config directives would go in config.

Is there a reason you're going to so much work to configure it to use the SCL version of nginx? If you just need a newer nginx, if you let the module manage the package, you'll get 1.10 or 1.11 on most platforms with a lot less fuss....

@wyardley
Copy link
Collaborator

wyardley commented Oct 9, 2016

This appears to be functioning as intended (the deprecation warning is appearing because params that were moved to nginx::config are being called via nginx::. I'm going to close out that issue; the docs issues and general structural issues are known and are being addressed elsewhere.

@wyardley wyardley closed this as completed Oct 9, 2016
@ubellavance
Copy link
Author

Ok, I see. My primary goal is to make sure that it works, not avoid the warning. Are you meaning that either should work? This is not what I experimented...

The reason for using the SCL version of nginx is to get support directly from Red Hat through our subscription. On Red Hat Enterprise Linux, nginx is not available in the base distro. It is available through EPEL, SCL, or Nginx's repo, but only SCL versions are supported by Red Hat support. Considering that I begin with nginx, that I have many non-standard requirements and the very good support at Red Hat, I don't mind putting the extra effort to use the SCL version. As a side effect, it helps testing this module with special parameters.

@wyardley
Copy link
Collaborator

wyardley commented Oct 9, 2016

Yes, fixing the warnings is safest, and will also make your configs more consistent.
So, at this point, do you understand how to tell which parameters are in which namespace?

@ubellavance
Copy link
Author

I understand that many, if not all parameters can be called from any of the two namespaces, but if the nginx:: namespase is used instead of nginx::config, it throws a warning but it works. I'll try to clean up my params so that the warning disappears, especially since I'd like to use my hiera files as examples for this module's documentation. The goal of most of the issues I have created is to make sure that I understand the module correctly so that I can contribute, at least for documentation.

@ubellavance
Copy link
Author

By the way this URL returns a 404 (and it's in the code).

@wyardley
Copy link
Collaborator

wyardley commented Nov 7, 2016

@ubellavance: First off, read the last paragraph, as all of this stuff will be rendered moot by the next release.

That said, I think you're not understanding. You needed to be moving things in the other direction for the version you're using, and based on what I said before. That is, almost nothing should be in nginx namespace, except the parameters that directly affect how the module works (e.g., manage_repos). So things like your example in #895:

nginx::config::gzip_types:
nginx::config::log_dir

should have the ::config there (and were probably implemented after the change was made, which is why they don't exist at the top level namespace). Virtually everything in your example above should be in nginx::config. Declaring an instance of nginx::config, but defining some variables outside of it may cause unpredictable results.

As the code stands now, if you don't declare nginx::config at all, it will try to setup an instance of nginx::config, and also give you a deprecation warning. However, if you are declaring it, I don't think the other parameters will pass through.

That said, in the recent change we made that will hit the next release (hopefully to be released within the next week), the nginx::config namespace is completely private, that is, you will have to use the nginx:: namespace only for all parameters, which should simplify things greatly. There's no soft fail or deprecation warning.

@wyardley
Copy link
Collaborator

wyardley commented Nov 7, 2016

By the way this URL returns a 404 (and it's in the code).

Yeah, that doc is now outdated by the update I mentioned above; a side effect of that is that it's no longer in master, so not visible via github without going back in time. However, I think the references to it will be gone in the newest releases as well.

@ubellavance
Copy link
Author

ubellavance commented Nov 8, 2016

You are right, I don't understand... And thanks for trying to explain. I have looked a the code of the module and I can't understand many parts of it. I spend about 5% of my time working with puppet and I'm not a programmer so I do my best to contribute by reporting what I find. If I understand correctly, I should wait the next release before reporting anything that is related to the nginx::config, right?

@ubellavance
Copy link
Author

If I understand correctly, #950 should get the result I'm expecting? Is there a release planned soon? I saw that a fair amount of work has been done since the last release. Good work!

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

No branches or pull requests

2 participants