Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Use `dns_name` instead of `name` for resource attribute #17

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants

The name resource attribute overrides the original name given to the resource. There is a special name_attribute which should be used for this instead (and is used in this PR).

See: sethvargo/chefspec#356 (comment)

@aetrion @dje would be nice if this could be merged (and released). Thanks.

pinging @aeden to see if we can get some action going 🍰 🎊

Owner

aeden commented Mar 17, 2014

@dje any objections to merging this in?

Contributor

josacar commented Mar 17, 2014

If so, major version must be bumped as it breaks backwards compatibility.

@josacar agreed. But the current implementation is broken though. If you send a notify action to a resource that you named my_resource, that notification wouldn't work, because during the compile phase, the resource was renamed to the value you entered in the name attribute.

Given that the name attribute is required in this resource, there is no way to have your resource be named something else.

This can be confusing, and difficult to track down. Furthermore, it's not how Chef resources are supposed to work.

Owner

aeden commented Mar 19, 2014

Pulling in @martinisoft, @therobot and @portertech to see if any of them want to weigh in, since they're all working with us right now and may be able to merge this PR.

Interesting, I have never run into this issue, as I tend to use the record name as the resource name, dnsimple_record "foobar.dnsimple.com", so I do not have issues notifying the resources. Having dns_name be the name_attribute addresses most backwards compatibility concerns, as long as users are following the pattern/practice that I do. I would have thought record_name would be more appropriate.

Contributor

josacar commented Mar 19, 2014

I usually do the same when resources have name attribute too. I've never redeclared name inside the resource block, but I'm fine with changing this.

Contributor

josacar commented Mar 19, 2014

@JeanMertz Can you write some tests to reflect the new syntax? Existing tests can be changed to reflect this, but as we are keeping backwards compability, IMHO I think some new tests can be better to reflect both styles.

At our company we make it a practice to always add the name attribute explicitly to the resource. This is to prevent the confusion as to what the name attribute actually describes. It is a bit more verbose, but also adds to the readability/understanding of some of the more obscure resources (according to us anyway).

Here's our (crude) Foodcritic rule to enforce this internally:

rule 'KABISA005', 'Avoid implied name attributes (only words allowed)' do
  tags %w[KABISA005 style resources]
  recipe do |ast|
    find_resources(ast).reject do |resource|
      join_in_name = false
      res = resource_name(resource, return_expressions: true)

      if res.respond_to?(:xpath)
        res_orig = res

        # any `File.join` inside the resource name is a smell
        join_in_name = res_orig.xpath(%q{.//const[@value='File']/
          ancestor::method_add_arg/descendant::ident[@value = 'join']
        }).any?

        # find any direct string inside the resource name argument
        string_refs = res_orig.xpath(%q{.//args_add/binary|string_literal})
          .children.xpath(%q{descendant::tstring_content})

        string_refs.each { |node| res << node }
      else
        res = [{ 'value' => res }]
      end

      # only allow "words" as resource name
      !join_in_name && res.all? { |string| string['value'] =~ /^[\w-]+$/ }
    end
  end
end

I will see about writing some tests somewhere this week. Thanks for the input.

@josacar I renamed dns_name to record_name as per @portertech's suggestion, it does sound more logical.

I am not sure about adding tests. What do you want to test? This is basic Chef functionality, so we would be testing Chef internals, I don't think that's ever a good practice.

Also, there are no old tests using this syntax, as the tests step into the LWRP, and work directly with the methods defined inside the LWRP.

Misread the source code, changed (some of) the existing tests to use the new record_name attribute.

While at it, I also created #19, using Rubocop and Foodcritic to further improve the code.

Contributor

josacar commented Apr 21, 2014

I was saying keep both ways of calling these LWRP, but if we only support the last one for me it's 👍

@josacar I only changed two of the fixtures, so the others still use the name attribute, and are still passing the test.

This one for example doesn't define the record_name option: https://github.com/kabisa-cookbooks/chef-dnsimple/blob/dont_use_name_attribute/test/fixtures/cookbooks/dnsimple_test/recipes/create_record.rb

Contributor

josacar commented Apr 21, 2014

@JeanMertz Okey, I didn't realize about that. So, 👍 to merge this.

Anyone interested in merging this?

Contributor

martinisoft commented Sep 19, 2014

@JeanMertz I'll take a look and review this PR by the weekend. Going to do a bunch of merges at once before cutting the new release.

Contributor

martinisoft commented Oct 17, 2014

@JeanMertz I'm interested in merging this PR. Can you merge in the current master and I will see about cutting a new release shortly after merging. Definitely agree about renaming that part of the resource so I'd like to ship this as a whole new version since it's potentially a breaking change.

Contributor

martinisoft commented Jan 8, 2016

Due to lack of activity here I'm going to close this pull request. You are welcome to re-open it with a rebased branch to propose this merge again. Thank you once again for your submission @JeanMertz ❤️

@martinisoft martinisoft closed this Jan 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment