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

Hardcoded checksums in library helpers #350

Closed
michaelklishin opened this issue Jul 21, 2015 · 8 comments

Comments

Projects
None yet
4 participants
@michaelklishin
Copy link
Contributor

commented Jul 21, 2015

If the checksum isn't specified, it is assumed to be one of the hardcoded values for an unknown version.

At the very least it should be documented what version it is and what checksum/hashing function is used.

michaelklishin added a commit to michaelklishin/cookbook-elasticsearch that referenced this issue Jul 21, 2015

@martinb3

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2015

Hi there -- the unknown reason is that it's intended to be a default value, but because the packages are distro specific, there isn't one good default value to put in the resource (it varies by platform).

@martinb3

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2015

I'll work on documenting that the method is intended to supply default values for a specific platform. Do you think there's a better way to do that?

I feel like having resource parameters for 'deb checksum' and 'rpm checksum' feels like too much for a resource that might not even represent a package install. After having consulted the Chef listserv, the most common response back I got was something to the effect of "hide the platform specifics in a helper library."

@martinb3 martinb3 added the doc label Jul 22, 2015

@martinb3 martinb3 added this to the 1.0.x bugfix release milestone Jul 22, 2015

@lfjewett

This comment has been minimized.

Copy link

commented Jul 22, 2015

The only problem for me is that the checksum is not defined in the docs as being a sha256sum, when the Elastic.co website contains sha1sum's on their download page. It is not at first apparent what checksum is needed.

@martinb3

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2015

@lfjewett I think Chef itself only uses sha 256 checksums everywhere, but I agree it would be good to make it explicit. Maybe @karmi can also see if they'd post the Sha 256 checksums to Elastic.co's website, too.

@michaelklishin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2015

There are several confusing parts I've identified:

  • Chef uses SHA-256, elasticsearch.org uses SHA-1
  • Even long time Chef users forget what hashing function is used.
  • There's a default checksum for the default version (which is fine) but since this is a library cookbook and not attribute-driven, it is more difficult to find what version is installed by default.

Not to turn this into a general 1.0 cookbook discussion but in my opinion this cookbook goes a bit too far in not using attributes, and the need to read cookbook code to investigate checksum failures when only package version is specified is just one example of that.

@martinb3

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2015

Perhaps it would be worth adding back in some basic recipes that use node attributes to declare the resources. @karmi, what do you think about supplying light weight wrapper recipes that just pass node attributes to the libraries?

E.g. something like:

node.override['elasticsearch']['foo'] = 'bar'
include_recipe 'elasticsearch'

Would end up running this resource:

elasticsearch_install 'elasticsearch' do
  foo node['elasticsearch']['foo']
end
@bdclark

This comment has been minimized.

Copy link

commented Sep 4, 2015

The wrapper cookbook I'm currently building uses a checksums attribute with a Mash of known checksums to lookup by version/type. Eg.

default['elastic']['checksums']['1.4.5']['deb'] = '68dce951181e9802e94fd83b894f4b628394fc44bb01c77eb61fdbd1940d94b5'
default['elastic']['checksums']['1.4.5']['rpm'] = '29b005c4148036556f78d6bd01a5e7c8e4ea60e2c20f82b63d8362ab46d83a19'
default['elastic']['checksums']['1.4.5']['tar'] = 'dc28aa9e441cbc3282ecc9cb498bea219355887b102aac872bdf05d5977356e2'
default['elastic']['checksums']['1.5.0']['deb'] = '15a02a2bea74da2330bb78718efb3a8f83a2b2e040a6ee859e100a6556981f36'
default['elastic']['checksums']['1.5.0']['rpm'] = 'b72a9fb9a2c0471e8fe1a35373cdcfe39d29e72b7281bfccbdc32d03ee0eff70'
default['elastic']['checksums']['1.5.0']['tar'] = 'acf572c606552bc446cceef3f8e93814a363ba0d215b323a2864682b3abfbe45'
default['elastic']['checksums']['1.6.2']['deb'] = 'da22c2df44ade970c7d8ec346fd37a440ed40f9e73fb3427b9eacf44baf298c2'
default['elastic']['checksums']['1.6.2']['rpm'] = '60c75a6f386bd729c6af2995af0a5290dd0ed6286673d435fc52620721815d91'
default['elastic']['checksums']['1.6.2']['tar'] = 'b7ef3aae0a263c2312bd1a25b191c3c108c92d5413c3527d776587e582c518d0'
default['elastic']['checksums']['1.7.1']['deb'] = '5832808f2a4c77de5af225db11da00138d9ea07a17f7b9212f4d2aac5317169d'
default['elastic']['checksums']['1.7.1']['rpm'] = '9b083f441490b32f98f9bf654e5e8ddec885838128726691103cfda5068423bc'
default['elastic']['checksums']['1.7.1']['tar'] = '86a0c20eea6ef55b14345bff5adf896e6332437b19180c4582a346394abde019'

Perhaps the same idea could be accomplished within this cookbook, either using node attributes or hard-coded in a helper method?

@martinb3 martinb3 closed this in be5633a Sep 15, 2015

@martinb3

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2015

@bdclark thank you for the suggestion. I've implemented something like that, for the elasticsearch_install provider to draw from as default values.

@michaelklishin:

  • I've added comments indicating SHA-256 next to all of the examples, to help document that.
  • I've moved the default checksums and URLs into an attributes file, and added a hint to the resource pointing users to that same attributes file
  • I've added a comment about how to lookup default version (go to the resource file instead of the attribute file)

I'm torn about adding an attribute to control the default version, as that makes things significantly more confusing, e.g. if I use the elasticsearch_resource and don't specify a version, does it determine the version to install from a node attribute or from a resource parameter? But I don't want to make the default version nil if you're using the resource as I feel like that also hides the value from some user base.

martinb3 added a commit that referenced this issue Sep 15, 2015

Expose def. version, url, and checksum as attr
RE: #350 - expose attributes for version, download url, and checksum. Providers will default to using these values.

Fixes #366.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.