Fix alternate path profile chaining #2121
Conversation
trevor-vaughan
force-pushed
the
deep_profile_chaining_fix
branch
3 times, most recently
from
5f0ef4b to
0f22468
Compare
trevor-vaughan
changed the title
There was a problem hiding this comment.
Thanks for the PR, @trevor-vaughan! Unfortunately I don't think it will work for absolute paths in dependencies. I've left you a comment with a potential alternate solution that you should test to see if it meets the needs of the issue you're trying to solve. Thanks!
| req_path = opts[:cwd] | ||
|
|
||
| if dep[:path] | ||
| req_path = File.join(req_path, dep[:path]) |
There was a problem hiding this comment.
I think this will only work if the path in the requirement is a relative path. What if I have an absolute path in my metadata?
depends:
- name: foo
path: '/path/to/foo'If my cwd is /home/aleff/profiles, this would set the req_path to /home/aleff/profiles/path/to/foo which won't exist... correct?
You probably want to look into using File.expand_path here instead which handles both relative and absolute path expansion.
[1] pry(main)> Dir.pwd
=> "/Users/aleff/profiles/test1"
[2] pry(main)> File.expand_path('../test2', Dir.pwd)
=> "/Users/aleff/profiles/test2"
[3] pry(main)> File.expand_path('/opt/chef/embedded', Dir.pwd)
=> "/opt/chef/embedded"
trevor-vaughan
force-pushed
the
deep_profile_chaining_fix
branch
from
aeb65ca to
f0ec079
Compare
|
@adamleff Good call. Fixed |
| new(dep[:name], | ||
| dep[:version], | ||
| cache, | ||
| File.absolute_path(req_path), |
There was a problem hiding this comment.
File.expand_path here should do the trick, right? We probably don't even need to call on File.absolute_path anymore.
There was a problem hiding this comment.
Heh, completely missed that. Updated.
Update to fix how multiple relative profile chaining functions. Closes inspec#2120 Signed-off-by: Trevor Vaughan <[email protected]>
trevor-vaughan
force-pushed
the
deep_profile_chaining_fix
branch
from
f0ec079 to
875831d
Compare
There was a problem hiding this comment.
This looks good to me, @trevor-vaughan, thanks!
In the future, we prefer it if you don't rebase/amend your commits each time as it helps us look at only the stuff that's changed between reviews. While it's not as big of a deal on this change since it's so small, it helps reduce the amount of time spent reviewing the PR on larger changes.
Thanks again for your contribution!
|
@adamleff Got it, I'll keep that in mind in the future. |
Update to fix how multiple relative profile chaining functions.
Closes #2120