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

use Chef::JSONCompat.parse for file_contents #2433 #2482

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@sawanoboly
Contributor

sawanoboly commented Nov 25, 2014

@lamont-granquist

This comment has been minimized.

Contributor

lamont-granquist commented Nov 25, 2014

yeah, chef-zero was written without "create_additions" or the "json_class" auto-inflation, all of the Chef::JSONCompat calls that it uses should have been #parse and not #from_json.

if there are any remaining calls to #from_json in chef-zero those are all bugs and need to be replaced (and #from_json should be hunted down and eradicated, its awful backcompat with insecure JSON usage).

@sawanoboly

This comment has been minimized.

Contributor

sawanoboly commented Nov 25, 2014

@lamont-granquist

Can I leave it up to you? Finding all bugs is difficult to me.

@jkeiser

This comment has been minimized.

Member

jkeiser commented Nov 25, 2014

The best way to hunt it down would actually be grep -R lib from_json :)

@lamont-granquist

This comment has been minimized.

Contributor

lamont-granquist commented Nov 25, 2014

just in lib/chef/chef_fs in this case -- unwinding the other uses of it is more annoying.

looks like this is the only one though.

@jkeiser

This comment has been minimized.

Member

jkeiser commented Nov 25, 2014

In which case, 👍. Is there already a bug for from_json genocide?

@lamont-granquist

This comment has been minimized.

Contributor

lamont-granquist commented Nov 25, 2014

no, its just well known pain from our past that hasn't been addressed yet.

@jkeiser

This comment has been minimized.

Member

jkeiser commented Nov 25, 2014

Actually, we really need a test for this (something that fails without your patch, and succeeds with it). What exactly is going wrong? (To be clear, this is a positive change: from_json without any arguments is a terrible, horrible idea and I'm unclear how it got in there in the first place.)

And if you don't mind adding a CHANGELOG.md entry while you're at it, that would be great.

@sawanoboly

This comment has been minimized.

Contributor

sawanoboly commented Nov 26, 2014

I have updated spec 6c0b6d2.

case: from_json causes exception in node#json_create.

[2014-11-26T14:27:10+09:00] ERROR: #<NoMethodError: undefined method `each' for nil:NilClass>
/Users/sawanoboriyu/github/higanworks/chef/lib/chef/node.rb:452:in `json_create'
/Users/sawanoboriyu/github/higanworks/chef/lib/chef/json_compat.rb:85:in `map_to_rb_obj'
/Users/sawanoboriyu/github/higanworks/chef/lib/chef/json_compat.rb:72:in `from_json'
/Users/sawanoboriyu/github/higanworks/chef/lib/chef/chef_fs/file_system/chef_repository_file_system_entry.rb:68:in `minimize'
/Users/sawanoboriyu/github/higanworks/chef/lib/chef/chef_fs/file_system/chef_repository_file_system_entry.rb:61:in `write'
@sawanoboly

This comment has been minimized.

Contributor

sawanoboly commented Dec 2, 2014

@jkeiser @lamont-granquist

Hi, any update on this?
I hope that fix local mode behavior before release version 12.0.0 GA.

@jkeiser

This comment has been minimized.

Member

jkeiser commented Dec 2, 2014

👍

1 similar comment
@sawanoboly

This comment has been minimized.

Contributor

sawanoboly commented Dec 2, 2014

👍

@lamont-granquist

This comment has been minimized.

Contributor

lamont-granquist commented Jan 23, 2015

dont we still need this it hasnt been merged?

@sawanoboly

This comment has been minimized.

Contributor

sawanoboly commented Jan 23, 2015

@lamont-granquist

Oh, this fix has been included on e809bb4 by @jkeiser (but without no specs 💢 ) , and already shipped.

I seemed that this PR doesn't have to keep state open.

@lamont-granquist

This comment has been minimized.

Contributor

lamont-granquist commented Jan 23, 2015

Heh, and git is smart enough to resolve the merge conflict from the identical edit.

The specs are still useful, like to get those merged so we don't ever regress.

@lamont-granquist lamont-granquist referenced this pull request Jan 27, 2015

Merged

Lcg/merges #2823

@lamont-granquist

This comment has been minimized.

Contributor

lamont-granquist commented Jan 27, 2015

closed by #2823

@sawanoboly

This comment has been minimized.

Contributor

sawanoboly commented Jan 28, 2015

Thanks! 👍

@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.