Stop storing state in attributes

[patcon]

Addresses #60

[legal90]

I actually like how you have changed get_artifact_url and get_artifact_checksum helpers. :+1:

But removing set_attributes_from_version recipe, we'll make this cookbook pretty hard to use, because ['confluence']['url'] and ['confluence']['checksum'] wouldn't be defined (they will be nil).

I think we should keep manage their default values for convenience:

# recipes/set_attributes_from_version.rb

node.default['confluence']['url'] = get_artifact_url
node.default['confluence']['checksum'] = get_artifact_checksum

[patcon]

:)

Ah. fixed some typs I noticed when I just ran a test-kitchen converge. Sorry for wasting anyone's time who tested before me.

@legal90 My sense was that computed attributes were an anti-pattern, but I'm open to whatever the consensus is. To confirm, was it clear that the helper method itself takes into account when the attr is set, and that takes precedence?

Ref: https://stackoverflow.com/questions/22844080/chef-use-library-function-in-attributes (sethvargo has done a lot of chef, and he seems to be saying computed attributes are a bad idea?)

[legal90]

We do not try to access library in the attribute file, we do that in a recipe, which is not so bad. Anyway, I agree that it is not a good idea, but we don't have another choice here. What we should not to do is forcing users to call our tricky libraries outside this cookbook. Let it be our internal stuff and let this cookbook just works.

[patcon]

but we don't have another choice here.

I'm not sure I follow. Setting node['confluence']['url'] at any precedence would override the computed value, right?

https://github.com/patcon/chef-confluence/blob/feature/GH-60-no-state-in-attrs/libraries/helpers.rb#L88

Or do you mean, if someone wants to fetch the url or checksum from within a wrapper? I suppose then they'd need to use the library, but I can't think of a normal reason someone would need these in a wrapper anyhow

[gsreynolds]

Hi! You might want to refer to this blog post by @coderanger that mentioned derived/computed attributes: https://coderanger.net/chef-tips/. Tip 5 would seem to be relevant to the discussion, specifically delaying interpolation of derived attributes until after all attribute overrides have been processed.

[legal90]

@patcon I just mean that we have to set the default values of ['confluence']['url'] and ['confluence']['checksum'] attributes, as it is implemented now.

This PR removes that logic, so it means that user will have to set mentioned attributes on some other levels or call our helpers from it's custom wrapper cookbook. Both of these ways are bad.

[legal90]

@gsreynolds Thanks. I've read this tip from coderanger long time ago and it really could be used in some custom cookbooks. But in this application cookbook it is not acceptable, at least because this approach doesn't allow users to override such attributes on other levels or in their wrapper cookbooks.

[patcon]

I'm unfortunately still not following the logic @legal90, but after this, I'll concede (for lack of understanding the problem :)

As a final explanation, the below wrapper would work fine. ie no need to use any other precedence level except default, and no need to call helpers to override attrs any differently than any chef user would expect. (I promise that's my last attempt at beating the dead horse in understanding one another!)

my_confluence wrapper cookbook

# attributes/default.rb
default['confluence']['url'] = "http://example.com/blah.tar.gz"

# recipes/default.rb
include_recipe "confluence::default"

confluence application cookbook

# attributes/default.rb
default['confluence']['url'] = nil

# recipes/default.rb
remote_file "/path/to/file.tar.gz" do
  source get_artifact_url
end

[legal90]

Ah, I'm so sorry, Patrick! I've missed this hunk where you have changed linux_installer and linux_standalone recipes. Seems like I was too tired these days :sleeping:. That's my bad...

Sure, your PR looks good and I'm gonna merge it. Thank you for explanation and sorry again!

[patcon]

haha ah, i definitely neglected that change in the first commit, so you aren't crazy for missing it :)

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.