Chef install now works when lockfile alone exists #1292 #131
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Issue #1292: chef install requires .rb file present #1292:
Issue located in the Chef CLI tool. The install.rb script only checked
for the presence of a policy file with an .rb extension whether or not
the user entered a policy lockfile. Changed code to check for .rb and
.lock.json extensions in the policy file path, and then verify the
existence of a policy file with that extension.
Updated error message in storage_config_spec.rb to correctly address
both .rb and .lock.json extensions.
Created test for new error language, and created test to verify error
when a policy file name is given, but the file does not exist.
DCO:
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the
best of my knowledge, is covered under an appropriate open
source license and I have the right under that license to
submit that work with modifications, whether created in whole
or in part by me, under the same open source license (unless
I am permitted to submit under a different license), as
Indicated in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including
all personal information I submit with it, including my
sign-off) is maintained indefinitely and may be redistributed
consistent with this project or the open source license(s)
involved.
Signed-off-by: Rick Morrow <rclark.morrow@gmail.com>
rclarkmorrow
force-pushed
the
ws-1292/rcm
branch
from
19569ee
to
6e699c5
Compare
| # See card CC-232 | ||
| # TODO: suggest next step. Add a generator/init command? Specify path to Policyfile.rb? | ||
| # See card CC-232 | ||
| if @policyfile_rel_path.end_with?(".lock.json") && !File.exist?(policyfile_lock_expanded_path) |
There was a problem hiding this comment.
My only complaint on this PR - and it is minor - is that it feels odd to create an instance variable just for these checks. Couldn't we check the ending of the return value from policyfile_lock_expanded_path?
There was a problem hiding this comment.
I am not sure if this is the right answer, however, I am fairly positive that "policyfile_lock_expanded_path" will always end with ".lock.json" and "policfile_expanded_path" will always end with ".rb" regardless of what file the user passes with the install command. Checking on "policyfile_rel_path" does check against what the user passes with the install command.
marcparadise
requested changes
Sep 16, 2020
There was a problem hiding this comment.
Thanks for doing this work @rclarkmorrow, and welcome to Chef Workstation!
This change looks good, and your reasoning around creating an instance var for relative path makes sense. I left one follow-up inline with this review. Once you update for that, I think we'll be in good shape to merge.
Adding additional test per comment: "Because it's a separate
validation path, this test should also be done for when a lockfile
is given."
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the
best of my knowledge, is covered under an appropriate open
source license and I have the right under that license to
submit that work with modifications, whether created in whole
or in part by me, under the same open source license (unless
I am permitted to submit under a different license), as
Indicated in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including
all personal information I submit with it, including my
sign-off) is maintained indefinitely and may be redistributed
consistent with this project or the open source license(s)
involved.
Signed-off-by: Rick Morrow <rclark.morrow@gmail.com>
rclarkmorrow
force-pushed
the
ws-1292/rcm
branch
from
eba54be
to
4dca444
Compare
marcparadise
approved these changes
Sep 16, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The install.rb script in the Chef CLI tool checked for the present of a policy file ending with
an .rb extension even when a lockfile (.lock.json) was passed as an argument. This produced
an error when a user tried to install from a .lock.json file.
Changes:
with that extension.
extension.
is passed as an argument, but doesn't exist.
Types of changes:
Checklist: