Add prototype of inspec.lock #949

stevendanna · 2016-08-22T13:20:34Z

This adds a basic prototype of inspec.lock. When the lockfile exists on
disk, the dependencies tree is constructed using the information in the
lock file rather than using the resolver.

Signed-off-by: Steven Danna steve@chef.io

ksubrama · 2016-08-22T21:47:15Z

lib/inspec/profile.rb

+                    end
+    end
+
+    def generate_lockfile(vendor_path = nil)


Comment: Make this an in-memory operation that doesn't render the lock file to disk.

ksubrama · 2016-08-22T21:58:58Z

So far so good. This makes sense as the "in progress" code that you walked me through this afternoon. I'm sure a lot of it will be refactored/rewritten soon - hence why most of my comments have a "Comment:" tag in them to denote that I'd simply like you to write a comment to that effect in there in case we don't get around to fixing that bit (you can never be too paranoid). I am 👍 on this.

chris-rock · 2016-08-23T12:40:21Z

lib/fetchers/zip.rb

+      if parent
+        parent.url
+      else
+        'file://target'


should this be "file://#{target}"?

chris-rock · 2016-08-23T12:51:41Z

I think the inspec.lock is not generated in the right path. It should be generated within the root profile directory

$ b inspec vendor test/unit/mock/profiles/dependencies/inheritance 
$ cat test/unit/mock/profiles/dependencies/inheritance/inspec.lock
cat: test/unit/mock/profiles/dependencies/inheritance/inspec.lock: No such file or directory
$ cat inspec.lock 
---
lockfile_version: 0
depends: []

chris-rock · 2016-08-23T12:52:39Z

lib/inspec/cli.rb

+  desc 'vendor', 'Download all dependencies and generate a lockfile'
+  def vendor(path = nil)
+    profile = Inspec::Profile.for_target('./', opts)
+    lockfile = profile.generate_lockfile(path)


👍 that this is part of the profile

maybe should call that profile.dependencies. The lockfile is only a written form of all the dependencies. The word lockfile is confusing on top of profile, because a profile should only know about dependencies but not lockfiles. I would attache the lockfile to DependencySet, instead of the call I would give it a path where the expected lock file should be located

This adds a basic prototype of inspec.lock. When the lockfile exists on disk, the dependencies tree is constructed using the information in the lock file rather than using the resolver. Signed-off-by: Steven Danna <steve@chef.io>

Signed-off-by: Steven Danna <steve@chef.io>

Eventually I think we'll want this as part of the fetcher API generally. Signed-off-by: Steven Danna <steve@chef.io>

Signed-off-by: Steven Danna <steve@chef.io>

chris-rock · 2016-08-23T13:00:14Z

lib/inspec/profile.rb

@@ -211,6 +212,44 @@ def locked_dependencies
      @locked_dependencies ||= load_dependencies
    end

+    def lockfile_exists?


I think we should hand the whole lockfile methods into the Lockfile class and just call it from there. Resolved lockfiles result in Dependencies. I think a profile should not know about lockfiles, only dependencies

I agree we need to increase the separation between Lockfile and Profile. @ksubrama and I discussed that a bit and I think the work to change how profile loading works will drive some of that out. However, in terms of the lockfile_exists? method specifically, I'm not sure it is true that we don't want profile knowing about lockfiles at all since the lockfile now lives in the profile directory.

I am not fan of introducing side-effect to classes that should work independently. We should at least bundle that into the dependency implementation instead of the profile implementation

It really isn't clear to me how lockfile_exists?() is a side-effect? Whether a lockfile is inside the profile is a property of the profile and depends on information (the path to the profile) that you don't necessarily want to expose to users of the class.

stevendanna · 2016-08-23T13:01:27Z

I think the inspec.lock is not generated in the right path. It should be generated within the root profile directory

See my comment in-line re this. Currently vendor assumes you are in a profile.

chris-rock · 2016-08-23T13:04:22Z

lib/inspec/dependencies/lockfile.rb

+
+    def self.from_file(path)
+      parsed_content = YAML.load(File.read(path))
+      version = parsed_content['lockfile_version']


cool to have that built in from the very beginning 👍

chris-rock · 2016-08-23T13:08:38Z

Thanks @stevendanna this works:

$ cd test/unit/mock/profiles/dependencies/inheritance
$ inspec vendor 
$ cat inspec.lock 
---
lockfile_version: 0
depends:
- name: profile_a
  resolved_source: file://../profile_a
  version_constraints: ">= 0"
  dependencies:
  - name: profile_c
    resolved_source: file://../profile_c
    version_constraints: ">= 0"
- name: profile_b
  resolved_source: file://../profile_b
  version_constraints: ">= 0"
- name: os-hardening
  resolved_source: https://github.com/dev-sec/tests-os-hardening/archive/master.tar.gz
  version_constraints: ">= 0"
  content_hash: d6c5299a7cdcce2040c157cc81cb2b21a85d3001493f8f3fb2aba175b9aa046c

chris-rock · 2016-08-23T13:25:06Z

discussed with @stevendanna, we are going to address my comments in later PRs

stevendanna · 2016-08-23T13:27:11Z

@chris-rock And I had an in-person discussions where we agreed that:

The high-level coordination of Inspec::Profile, Inspec::Lockfile, and Inspec::DependencySet isn't right yet. We think that the right path here will get clearer as we work on scoping profile loading.
The UX around the vendor command needs to be finalized. The path argument should probably be the path to the profile and then the lockfile should get placed at the same level as inspec.yml. The vendor directory can move to an option.

chris-rock · 2016-08-26T08:38:03Z

related to #888

chef-supermarket added the Signed CLA label Aug 22, 2016

stevendanna changed the title ~~Add prototype of inspec.lock~~ WIP: Add prototype of inspec.lock Aug 22, 2016

arlimus self-assigned this Aug 22, 2016

ksubrama reviewed Aug 22, 2016
View reviewed changes

chef-supermarket added the Signed CLA label Aug 23, 2016

stevendanna force-pushed the ssd/deps-lockfile branch from 170f47d to d0fc14c Compare August 23, 2016 08:47

chef-supermarket added the Signed CLA label Aug 23, 2016

stevendanna changed the title ~~WIP: Add prototype of inspec.lock~~ Add prototype of inspec.lock Aug 23, 2016

chef-supermarket added the Signed CLA label Aug 23, 2016

stevendanna force-pushed the ssd/deps-lockfile branch from 3e96221 to b246e11 Compare August 23, 2016 10:52

chef-supermarket added the Signed CLA label Aug 23, 2016

chris-rock force-pushed the ssd/deps-lockfile branch from b246e11 to c734c4c Compare August 23, 2016 12:35

chef-supermarket added the Signed CLA label Aug 23, 2016

chris-rock reviewed Aug 23, 2016
View reviewed changes

lib/fetchers/zip.rb

if parent

parent.url

else

'file://target'

Copy link

Contributor

chris-rock Aug 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be "file://#{target}"?

chris-rock reviewed Aug 23, 2016
View reviewed changes

stevendanna added 5 commits August 23, 2016 13:54

Add prototype of inspec.lock

9c1b82e

This adds a basic prototype of inspec.lock. When the lockfile exists on disk, the dependencies tree is constructed using the information in the lock file rather than using the resolver. Signed-off-by: Steven Danna <steve@chef.io>

Add comments based on code review and plans for future work

3049eb1

Signed-off-by: Steven Danna <steve@chef.io>

Don't set nil cwd in inspec/profile

a6ec345

Add archive_path helper to Tar and Url fetchers

02d611e

Eventually I think we'll want this as part of the fetcher API generally. Signed-off-by: Steven Danna <steve@chef.io>

Remove unused url functions from fetchers

d9b6210

Signed-off-by: Steven Danna <steve@chef.io>

stevendanna force-pushed the ssd/deps-lockfile branch from c734c4c to d9b6210 Compare August 23, 2016 12:55

chef-supermarket added the Signed CLA label Aug 23, 2016

chris-rock reviewed Aug 23, 2016
View reviewed changes

chris-rock merged commit 961e815 into master Aug 23, 2016

chris-rock deleted the ssd/deps-lockfile branch August 23, 2016 13:25

chris-rock modified the milestone: 0.32.0 Aug 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prototype of inspec.lock #949

Add prototype of inspec.lock #949

stevendanna commented Aug 22, 2016

ksubrama Aug 22, 2016

ksubrama commented Aug 22, 2016

chris-rock Aug 23, 2016

chris-rock commented Aug 23, 2016 •

edited

Loading

chris-rock Aug 23, 2016

chris-rock Aug 23, 2016 •

edited

Loading

chris-rock Aug 23, 2016

stevendanna Aug 23, 2016

chris-rock Aug 23, 2016

stevendanna Aug 23, 2016

stevendanna commented Aug 23, 2016

chris-rock Aug 23, 2016

chris-rock commented Aug 23, 2016

chris-rock commented Aug 23, 2016

stevendanna commented Aug 23, 2016

chris-rock commented Aug 26, 2016

Add prototype of inspec.lock #949

Add prototype of inspec.lock #949

Conversation

stevendanna commented Aug 22, 2016

Choose a reason for hiding this comment

ksubrama commented Aug 22, 2016

Choose a reason for hiding this comment

chris-rock commented Aug 23, 2016 • edited Loading

Choose a reason for hiding this comment

chris-rock Aug 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevendanna commented Aug 23, 2016

Choose a reason for hiding this comment

chris-rock commented Aug 23, 2016

chris-rock commented Aug 23, 2016

stevendanna commented Aug 23, 2016

chris-rock commented Aug 26, 2016

chris-rock commented Aug 23, 2016 •

edited

Loading

chris-rock Aug 23, 2016 •

edited

Loading