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

Interpolate %{path} in verify command #3693

Merged
merged 2 commits into from
Jul 23, 2015
Merged

Interpolate %{path} in verify command #3693

merged 2 commits into from
Jul 23, 2015

Conversation

margueritepd
Copy link
Contributor

See #3232

@coderanger
Copy link
Contributor

Can you add a comment indicating why there are two? Maybe that would give us a mild chance of remembering to remove the old one for Chef 13.

@ranjib
Copy link
Contributor

ranjib commented Jul 21, 2015

@coderanger RFC says the variable name should be path. current implementation uses file . Keeping file as it is will ensure nothing breaks downstream

@stevendanna
Copy link
Contributor

@ranjib I believe @coderanger was asking for a comment in the code for people who come across it later. I'm 👍 on having a code comment about why we have two.

@coderanger
Copy link
Contributor

Yes, we can't remove file until Chef 13, but we should eventually revisit to clean this up.

@ranjib
Copy link
Contributor

ranjib commented Jul 21, 2015

@margueritepd that helper method name should be something different, in itself its not a test. may be platform_specific_command ?
@coderanger i cant think of a way to throw deprecation warning for file usage, else it would have helped..
👍

@ranjib
Copy link
Contributor

ranjib commented Jul 21, 2015

@stevendanna ah .. got it now

@coderanger
Copy link
Contributor

Chef::Log.deprecate('%{file} is deprecated in favor of %{path}') if @command.include?('%{file}')

or something like that.

@margueritepd
Copy link
Contributor Author

@coderanger comment and deprecation warning added, tests updated :)

I didn't use Chef::Log.deprecation because that throws an error apparently.

@mcquin
Copy link
Contributor

mcquin commented Jul 21, 2015

@margueritepd there is a configuration option for treating deprecation warnings as errors. It's best to use Chef::Log.deprecation.

@margueritepd
Copy link
Contributor Author

@mcquin back to using deprecation!

@mcquin
Copy link
Contributor

mcquin commented Jul 22, 2015

👍

thommay added a commit that referenced this pull request Jul 23, 2015
…-to-use-path-as-variable

Interpolate `%{path}` in verify command
@thommay thommay merged commit 5daf096 into chef:master Jul 23, 2015
@margueritepd margueritepd deleted the issue-3232-template-verify-to-use-path-as-variable branch July 23, 2015 17:43
@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.

7 participants