Transplant verify functionality from chef-cli #479

marcparadise · 2019-09-10T18:27:43Z

This moves the verify command into chef-workstation, since it is only used to verify the builds in the omnibus pipeline. This helps reduce the indirect circular dependency between Workstation and CLI as well, since CLI no longer needs to know about what tools ship with Workstation.

This is minimally modified transplant of the Verify class and the supporting class ComponentTest. Once all is well, we can update chef-cli to remove the verify command.

Signed-off-by: Marc A. Paradise marc.paradise@gmail.com

Related Issue

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

I have read the CONTRIBUTING document.
I have run the pre-merge tests locally and they pass.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
All commits have been signed-off for the Developer Certificate of Origin.

afiune

I would like to talk about this PR. A general question would be

"What happens when a user tries to run chef verify?"

Are we deprecating this command? or just remove it for good without notice?

marcparadise · 2019-09-12T15:21:10Z

It was hidden internal command which wasn't provided for use to customers, so I'm planning to remove it without deprecation

afiune

Lots of code, it will take me a few to review this.

Do we need this file though?
omnibus/verification/spec/unit/fixtures/eg_omnibus_dir/valid/embedded/apps/chef-dk/.keep

Also it is hard to see what is copy paste code and what are the things you have modified, like commenting out code, etc.

omnibus/omnibus-test.ps1

omnibus/verification/verify.rb

omnibus/omnibus-test.sh

marcparadise · 2019-09-13T16:54:39Z

@afiune here's a diff of verify and component_test:

https://gist.github.com/marcparadise/3c7470aee219a0c9ae847ac98928e223

tas50 · 2019-09-14T00:17:55Z

.expeditor/verify.pipeline.yml

@@ -8,6 +8,15 @@ steps:
    executor:
      docker:
        image: ruby:2.6-stretch
+- label: verification-unit-tests-ruby-2.6


We should just call this component-tests. They're not unit tests really for a lot of things.

afiune · 2019-09-16T19:31:34Z

@marcparadise I just looked at the buildkite job output and I see the following:

All examples were filtered out; ignoring {:focus=>true}
  | ........true
  | true
  | true
  | .ruby -e 'puts Dir.pwd'
  | .ruby -e 'puts Dir.pwd'
  | .......exit 0
  | .exit 1
  | .exit 0
  | /usr/local/bin/ruby verify_me
  | .exit 0
  | /usr/local/bin/ruby verify_me
  | .exit 0
  | /usr/local/bin/ruby verify_me
  | .exit 0
  | /usr/local/bin/ruby verify_me
  | .exit 0
  | /usr/local/bin/ruby integration_test
  | .exit 0
  | .exit 0
  | /usr/local/bin/ruby verify_me
  | .exit 0
  | /usr/local/bin/ruby verify_me
  | .exit 0
  | exit 0
  | /usr/local/bin/ruby verify_me
  | /usr/local/bin/ruby verify_me
  | .exit 0
  | exit 0
  | /usr/local/bin/ruby verify_me
  | /usr/local/bin/ruby verify_me
  | .exit 0
  | .exit 0
  | exit 0
  | exit 0
  | /usr/local/bin/ruby verify_me
  | /usr/local/bin/ruby verify_me
  | /usr/local/bin/ruby verify_me
  | .exit 0
  | exit 0
  | exit 0
  | /usr/local/bin/ruby verify_me
  | /usr/local/bin/ruby verify_me
  | /usr/local/bin/ruby verify_me
  | .
  |  
  | Finished in 1.21 seconds (files took 0.92669 seconds to load)
  | 32 examples, 0 failures

https://buildkite.com/chef/chef-chef-workstation-master-verify/builds/610#125ed5e8-80f1-4cb9-9fd3-36acb27131d1

Wondering if thats the expected output and if the exit 1 should actually be a failure?

tyler-ball

@marcparadise Salim and I added some commits to help us understand what is going on here.

It looks to me like you wrote some scripts (run.rb, verify.rb, component_test.rb) that contain logic for how to test components included inside Chef Workstation. LGTM, 👍

The spec folder initially confused me until I realized those are the tests for the logic in verify.rb and other scripts. I added a short README to help my future brain remember.

With that understanding this looks 🚀 🍰 to me!

This moves the verify command into chef-workstation, since it is only used to verify the builds in the omnibus pipeline. This removes the indirect circular dependency between Workstation and CLI as well, since CLI no longer needs to know about what tools ship with Workstation. This is a minimally modified transplant of the Verify class and the supporting class ComponentTest from chef-cli. Once all is well, we can update chef-cli to remove the verify command. Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

This allows us to keep `verify.rb` containing only the verification class definition, which prevents us from actually running verification when attempting to run unit specs. Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

Moved inlined ruby into a separate script for readability. Removed chef-run because it's tested in verify itself. Removed a second `chef env` run because one verifies just as well as two in this case. Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

Because moving `verify` into Workstation doesn't alter which tests we do/do not run, we will address these separately. We'll be tracking the state of verification tests under #487. Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

marcparadise · 2019-09-18T15:23:24Z

Running CI again. The secondary issues we've been having lately have been bubbling through to these builds, but it looks like current master is sane.

marcparadise · 2019-09-18T15:33:25Z

Thanks for adding the README, I meant to include that when I made that directory - figured it would otherwise be weird -- "Why are these specs in a random omnibus directory..."

Signed-off-by: tyler-ball <tball@chef.io>

We've seen issues where "exit" does not properly set the system exit code but "exit!" does. Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

omnibus/omnibus-test.ps1

tas50 · 2019-09-18T21:09:36Z

Gemfile

@@ -3,6 +3,9 @@ source "https://rubygems.org"
 group :development do
  gem "chefstyle"
  gem "rake"
+  # enable tests for the verification behavior in omnibus/verification
+  gem "chef-cli"
+  gem "rspec"


If unit is disabled do we still need rspec?

Return to the original state of Workstation where we did not run unit tests in verification. There are a couple of problems with unit tests currently that need to be addressed before we enable them: - they must be run as root because we bundle install in the gem's chef-workstation-ruby source lib directory - windows unit tests are failing. We don't currently know yet if are legitimate failures, but we do know that we've been shipping without runnign those tests for a while now. Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

marcparadise requested review from a team as code owners September 10, 2019 18:27

marcparadise force-pushed the mp/own-verify branch 6 times, most recently from 891992d to dbb3f31 Compare September 12, 2019 14:51

afiune reviewed Sep 12, 2019

View reviewed changes

marcparadise requested a review from a team as a code owner September 12, 2019 16:36

marcparadise mentioned this pull request Sep 12, 2019

Remove chef verify hidden internal command. chef/chef-cli#30

Merged

10 tasks

marcparadise force-pushed the mp/own-verify branch 2 times, most recently from 7b8b436 to 55edea5 Compare September 12, 2019 16:55

afiune reviewed Sep 12, 2019

View reviewed changes

omnibus/omnibus-test.ps1 Outdated Show resolved Hide resolved

omnibus/verification/verify.rb Outdated Show resolved Hide resolved

omnibus/omnibus-test.sh Outdated Show resolved Hide resolved

afiune mentioned this pull request Sep 12, 2019

Typescript EventEmitter and Remove global variables chef/chef-workstation-app#79

Merged

10 tasks

marcparadise force-pushed the mp/own-verify branch from ae96cd6 to f1395af Compare September 13, 2019 16:54

tas50 reviewed Sep 14, 2019

View reviewed changes

marcparadise force-pushed the mp/own-verify branch from 8f9e1cc to 05998a4 Compare September 16, 2019 18:34

tyler-ball mentioned this pull request Sep 16, 2019

Workstation package verification tests are incomplete, failing #487

Open

tyler-ball added Aspect: Testing Does the project have good coverage, and is CI working? Status: Adopted An issue that is being worked on. Type: Chore non-critical maintenance of a project. labels Sep 16, 2019

afiune mentioned this pull request Sep 16, 2019

[DEPENDS ON #479] Bump chef-cli to 1.0.14 #503

Closed

This was referenced Sep 16, 2019

[DEPENDS ON #479] Bump chef-cli to 1.0.14 #504

Closed

Bump kitchen-vcenter to 2.5.2 #502

Closed

tyler-ball approved these changes Sep 17, 2019

View reviewed changes

marcparadise requested a review from a team as a code owner September 17, 2019 22:52

marcparadise force-pushed the mp/own-verify branch from d7f2e23 to df7f105 Compare September 18, 2019 15:19

marcparadise added 7 commits September 18, 2019 11:20

Invoke the verify class directly in omnibus-test

56e53c3

This allows us to keep `verify.rb` containing only the verification class definition, which prevents us from actually running verification when attempting to run unit specs. Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

Pull in tests for the verify behavior

3b829f1

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

Enable tests of omnibus/verify

9260c43

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

Clean up omnibus-test

f6d5dff

Moved inlined ruby into a separate script for readability. Removed chef-run because it's tested in verify itself. Removed a second `chef env` run because one verifies just as well as two in this case. Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

Remove chef-dk from fixtures, it is not used

df7f105

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

Adding short README explaining the new files

0e19e57

Signed-off-by: tyler-ball <tball@chef.io>

afiune approved these changes Sep 18, 2019

View reviewed changes

Use exit!, not exit

403698a

We've seen issues where "exit" does not properly set the system exit code but "exit!" does. Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

tas50 reviewed Sep 18, 2019

View reviewed changes

omnibus/omnibus-test.ps1 Outdated Show resolved Hide resolved

tas50 reviewed Sep 18, 2019

View reviewed changes

marcparadise force-pushed the mp/own-verify branch from 89fe850 to d5d5c8a Compare September 19, 2019 12:24

marcparadise merged commit cb93e1a into master Sep 19, 2019

chef-expeditor bot deleted the mp/own-verify branch September 19, 2019 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transplant verify functionality from chef-cli #479

Transplant verify functionality from chef-cli #479

marcparadise commented Sep 10, 2019 •

edited

Loading

afiune left a comment •

edited

Loading

marcparadise commented Sep 12, 2019

afiune left a comment

marcparadise commented Sep 13, 2019

tas50 Sep 14, 2019

afiune commented Sep 16, 2019 •

edited

Loading

tyler-ball left a comment

marcparadise commented Sep 18, 2019

marcparadise commented Sep 18, 2019

tas50 Sep 18, 2019

Transplant verify functionality from chef-cli #479

Transplant verify functionality from chef-cli #479

Conversation

marcparadise commented Sep 10, 2019 • edited Loading

Related Issue

Types of changes

Checklist:

afiune left a comment • edited Loading

Choose a reason for hiding this comment

marcparadise commented Sep 12, 2019

afiune left a comment

Choose a reason for hiding this comment

marcparadise commented Sep 13, 2019

tas50 Sep 14, 2019

Choose a reason for hiding this comment

afiune commented Sep 16, 2019 • edited Loading

tyler-ball left a comment

Choose a reason for hiding this comment

marcparadise commented Sep 18, 2019

marcparadise commented Sep 18, 2019

tas50 Sep 18, 2019

Choose a reason for hiding this comment

marcparadise commented Sep 10, 2019 •

edited

Loading

afiune left a comment •

edited

Loading

afiune commented Sep 16, 2019 •

edited

Loading