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

Document default values for all resources #348

Closed
martinb3 opened this issue Jul 17, 2015 · 5 comments
Closed

Document default values for all resources #348

martinb3 opened this issue Jul 17, 2015 · 5 comments

Comments

@martinb3
Copy link
Contributor

Document all simple default parameters for resource. Many resources have computed default parameters as well, so we should explain the computation for those. Don't forget to document the default action.

@astral303
Copy link

Needs more documentation changes and perhaps a big heads up or a warning for anyone upgrading from 1.0.0.

There are subtle changes that tripped me up, for example es_home used to be lazy { dir } but became a hardcoded value in elasticsearch_configure.

Also pid_path no longer auto-populates pid_file for _service, another nasty surprise. That was handy magic. I think the most common case is to override pid_path, not the filename. Pid_path is now only required so it could create the dir for pid_file.

Anyone using custom paths would do well to study commit d341834 and check their configuration.

@astral303
Copy link

Another gotcha for custom path users: path.conf, path.dir and path.log must be overridden in the configuration object itself as well as in the resource attributes, when using elasticsearch_configure, as they are no longer defaulted to resource values:

configuration ({
'path.conf' => es_path_conf,
'path.data' => es_path_data,
'path.logs' => es_log_root,
...

@martinb3
Copy link
Contributor Author

@astral303, gah... sorry if that broke you today. We got a lot of complaints about the magic as well as complaints about using lazy { } from poise; I guess it was preventing people from using two cookbooks that both wanted poise.

I was thinking I was maintaining backwards compatibility for most users; I didn't think the overrides were in wide use. I apologize again for that, and I hope you were pinned in such a way that it didn't cause an outage :(

I'm trying to strike a balance between 'magic' and obvious values. I guess Chef 12.5.x will support lazy { } in core Chef, and maybe I can revert some of these. Until then, though, would it be better to revert this behavior back? Maybe use nil to signal a lazy value, and then compute them in the provider?

@astral303
Copy link

Martin, no worries, we are pinned, I discovered this in testing. I am used to cookbook version upgrades having subtle changes, so it's no big deal.

I didn't realize there was a conflict between poise and lazy--that is certainly an important practical motivation.

I wonder whether we can take advantage of using a common name for ElasticSearch resources, where shared concerns like home location can be extracted into a shared hash or a shared block of attributes. Perhaps the ES configuration can be directly specified and values like path_dir can be extracted from that block, as looked up by the shared resource identifier.

I think your idea to compute the derived values is on point. "nil" with computation in the provider is a great alternative. For example, for pid_file, we could derive the pid_path by doing File.dirname() on pid_file.

@martinb3
Copy link
Contributor Author

@astral303 I'm about to release another bugfix version (v1.0.2) that should fix some of these by doing the nil default that causes the 'automatic' value to be used (both for the resource parameters and resource hash, in all of your examples). This should allow you to remove some of your added overrides you needed on v1.0.1.

I like the idea of using a common name for them, and drawing on previous resources' settings if the name matches. I've created #373 to track it.

My only concern with that approach is that things like elasticsearch_plugin 'head' would no longer work, as it would try to use head as the instance name instead of the plugin name.

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