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

Added source_file to FromFile #6938

Merged
merged 1 commit into from
Mar 6, 2018
Merged

Added source_file to FromFile #6938

merged 1 commit into from
Mar 6, 2018

Conversation

zealws
Copy link
Contributor

@zealws zealws commented Mar 3, 2018

Description

This adds a new field source_file to the FromFile mixin that contains the source file used to load the object.

This is useful if you want to trace back a Role/Cookbook::Metadata to the file it was generated from.

We have some scripts that do some custom validation on Role/Cookbook::Metadata objects and currently monkeypatch the FromFile class to contain this attribute, but it'd be nice to have upstream and get rid of the monkeypatch. They mostly use this information to provide meaningful error messages to users in the event that this custom validation fails.

I double-checked that none of the classes using FromFile were already using the name source_file for something else, so this won't conflict with any of the code in the chef repo. Happy to rename if desired.

I didn't update RELEASE_NOTES.d since I thought this was a small change, but I can do that too, if necessary.

Issues Resolved

None

Check List

@zealws zealws requested a review from a team March 3, 2018 00:11
@lamont-granquist
Copy link
Contributor

chefstyle issues that are making travis unhappy would need to get fixed.

this is one instance where i think i'd argue that self.source_file = needs to get used over @source_file = so if a subclass overrides the setter it'll just work.

kind of looks like class_from_file should be updated as well, although i have no idea how that gets used and maybe it isn't appropriate, IDK...

bit worried that this mixin may get used well outside of core chef as well, although now would be a good time to get it in for 14.0 then.

@thommay
Copy link
Contributor

thommay commented Mar 5, 2018

I think this is mergable once the chefstyle problems are fixed @zfjagann - thanks for your contribution to Chef!

@zealws
Copy link
Contributor Author

zealws commented Mar 5, 2018

I addressed the comments above, including chef style issues, using self.source_file= instead of @source_file=, and added support to class_from_file and associated tests.

bit worried that this mixin may get used well outside of core chef as well

This is also something I thought about, but can't easily address, since I don't have access to code outside of the chef codebase (or our own site's chef code). I did, however, set the source_file attribute before the {class,instance}_eval, so if there's a conflicting attribute in the file being loaded, it will win over the source_file implicitly being set, so the behavior shouldn't change in this case.

The only case where this would change behavior unexpectedly is if someone loaded a file which included source_file but the class receiving the data didn't include source_file (which would be invalid). In which case, after the change, the code would pass since source_file is now a valid setting but it wasn't before. This seems like a very unlikely circumstance, however.

This new field tracks the file from which the object was loaded.

Signed-off-by: Zeal Jagannatha <zealjagannatha@gmail.com>
@zealws
Copy link
Contributor Author

zealws commented Mar 5, 2018

I looked at the failing appveyor output and I wasn't able to determine what the problem was from the output (most of the output didn't look related to my PR). If it's a problem with this PR please let me know and I will fix it.

@thommay
Copy link
Contributor

thommay commented Mar 6, 2018

We think the intermittent appveyor problems are caused by the instance running out of memory; not your fault!

@lock
Copy link

lock bot commented May 5, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2018
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.

3 participants