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

Conversation

Projects
None yet
7 participants
@margueritepd
Contributor

margueritepd commented Jul 21, 2015

See #3232

@coderanger

This comment has been minimized.

Contributor

coderanger commented Jul 21, 2015

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

stevendanna commented Jul 21, 2015

@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

This comment has been minimized.

Contributor

coderanger commented Jul 21, 2015

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

@ranjib

This comment has been minimized.

Member

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

This comment has been minimized.

Member

ranjib commented Jul 21, 2015

@stevendanna ah .. got it now

@coderanger

This comment has been minimized.

Contributor

coderanger commented Jul 21, 2015

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

or something like that.

@margueritepd

This comment has been minimized.

Contributor

margueritepd commented Jul 21, 2015

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

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

@mcquin

This comment has been minimized.

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

This comment has been minimized.

Contributor

margueritepd commented Jul 22, 2015

@mcquin back to using deprecation!

@mcquin

This comment has been minimized.

Contributor

mcquin commented Jul 22, 2015

👍

thommay added a commit that referenced this pull request Jul 23, 2015

Merge pull request #3693 from margueritepd/issue-3232-template-verify…
…-to-use-path-as-variable

Interpolate `%{path}` in verify command

@thommay thommay merged commit 5daf096 into chef:master Jul 23, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@margueritepd margueritepd deleted the margueritepd:issue-3232-template-verify-to-use-path-as-variable branch Jul 23, 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.