-
Notifications
You must be signed in to change notification settings - Fork 682
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
Support vendored profiles in Habitat-packaged profiles #1594
Conversation
@@ -92,6 +106,20 @@ def verify_profile | |||
Habitat::Log.info('Profile is valid.') | |||
end | |||
|
|||
def vendor_profile_dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like we're duplicating code here: https://github.com/chef/inspec/blob/master/lib/inspec/base_cli.rb#L149 Can we extract one method to meet the needs of base_cli
and this implementation?
This vendor_deps
is also used https://github.com/chef/inspec/blob/master/lib/bundles/inspec-compliance/cli.rb#L160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While there's not much to it, I will create a Inspec::ProfileVendor
class right now that does will do the right thing and expose the necessary methods for the two implementations to get what they need.
I specifically can't reuse this implementation line-for-line because I need the vendored content in the actual profile path itself, not the working directory, etc. But I can work with it.
lib/fetchers/url.rb
Outdated
FileUtils.mv(temp_archive_path, final_path) | ||
FileUtils.chmod(0644, final_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't but has this change any impact on on-habitat environments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it shouldn't, but out of abundance of caution, I'll move it out of the fetcher and into the habitat bundle instead.
PR updated based on feedback from @chris-rock. @arlimus and @alexpop -- if either of you have time, I'd appreciate a set of eyes on this one. |
This change adds support in Habitat-packaged profiles for profiles that depend on other profiles. When `inspec habitat profile create` or `inspec habitat profile upload` is run, it will see if the profile's dependencies have been vendored yet, and if not, it will vendor them before creating the habitat artifact. For the git and URL fetchers, more explicit creation of the target directories for the vendored profiles is done. This is implicitly done via normal CLI interactions a user may go through, but in our case, we want to ensure those directories are there before the fetchers try to write out content. By adding this support, we also fix a bug experienced in Habitat where a profile that was packaged before an `inspec exec` was run for the profile would cause a failure in Habitat. This is caused by `inspec exec` doing a vendor of the dependencies if necessary and generating the inspec.lock file. In Habitat, the package dir is not writable by the hab user and InSpec would fail to run due to an inability to write out an inspec.lock. Signed-off-by: Adam Leff <[email protected]>
Per PR feedback, `Inspec::ProfileVendor` is created to centralize the logic and data of vendoring profile dependencies. The `BaseCLI` class and the `Habitat::Profile` class have been modified to use it Signed-off-by: Adam Leff <[email protected]>
The CLI output for the vendoring of profiles has been updated slightly to be more clear, and the functional tests have been modified to match as well. Signed-off-by: Adam Leff <[email protected]>
9465cd9
to
96d1843
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the new functionality, thank you @adamleff !!
Also great on the updated re-use + kudos for the review @chris-rock
lgtm 😁
This change adds support in Habitat-packaged profiles for
profiles that depend on other profiles. When
inspec habitat profile create
orinspec habitat profile upload
is run,it will see if the profile's dependencies have been vendored
yet, and if not, it will vendor them before creating the
habitat artifact.
For the git and URL fetchers, more explicit creation of the
target directories for the vendored profiles is done. This
is implicitly done via normal CLI interactions a user may
go through, but in our case, we want to ensure those directories
are there before the fetchers try to write out content.
By adding this support, we also fix a bug experienced in Habitat
where a profile that was packaged before an
inspec exec
was runfor the profile would cause a failure in Habitat. This is caused
by
inspec exec
doing a vendor of the dependencies if necessaryand generating the inspec.lock file. In Habitat, the package dir
is not writable by the hab user and InSpec would fail to run due
to an inability to write out an inspec.lock.