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

Pass name by knife cil attribute #3195

Closed

Conversation

sawanoboly
Copy link
Contributor

Hi,

We can't pass attribute which named name since this change #2338 via knife ssh.

I would like to fix it.

@ranjib
Copy link
Contributor

ranjib commented Apr 8, 2015

👍
chef_environment and other node attributes that are computed using method will have this issue as well.

/cc @chef/client-core

@sawanoboly
Copy link
Contributor Author

@ranjib Yes 😃

I wrote a spec for this issue to cover compatibility on future.

And I'll run integration test to avoid same problem with my plugin like below. This build is scheduled at every night by my heroku scheduler 👍

https://circleci.com/gh/higanworks/knife-zero/20

@@ -182,7 +182,7 @@ def extract_nested_value(data, nested_value_spec)
# `keys` - want the key named `keys`, not a list of
# available keys.
elsif data.respond_to?(:[])
data = data[attr]
data = data[attr] ? data[attr] : data.send(attr.to_sym)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is basically duplicating the next elsif clause ( elsif data.respond_to?(attr.to_sym) ). Would it be better to change the elsif clause on line 184 to data.respond_to?(:[]) && data.has_key?(attr) ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise we probably need to guard this with a respond_to? which would then completely duplicate the next elsif statement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed about

elsif data.respond_to?(:[]) && data.has_key?(attr)
  data = data[attr]
elsif data.respond_to?(attr.to_sym)
  data = data.send(attr.to_sym)

@sawanoboly
Copy link
Contributor Author

@danielsdeleo @tyler-ball

Thanks, that is better. I'll update my branch after a few hours.

@sawanoboly
Copy link
Contributor Author

I've updated code to reflect comments ☺️

@danielsdeleo
Copy link
Contributor

👍 from me. Would be nice to have a test that covers --attributes when the attribute doesn't exist and also there is no method with that name either. But I'd be okay to add that during merge.

@tyler-ball
Copy link
Contributor

👍

@btm
Copy link
Contributor

btm commented Apr 21, 2015

👍

I presume @danielsdeleo's comment means he's going to add a test and merge this?

@danielsdeleo
Copy link
Contributor

Merged w/ 182f376

danielsdeleo added a commit that referenced this pull request May 26, 2015
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants