Skip to content
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

New projects lacked critical files, such as kitchen and package scripts #867

Merged
merged 1 commit into from
Apr 8, 2019
Merged

New projects lacked critical files, such as kitchen and package scripts #867

merged 1 commit into from
Apr 8, 2019

Conversation

iamjohnnym
Copy link
Contributor

@iamjohnnym iamjohnnym commented Jan 7, 2019

Description

Related: #831

It looks like #820 prevented omnibus from creating some critical files. Dir.glob() does not see hidden files by default. Since the kitchen files were failing, any instructions after were failing to execute, which led to package-scripts from also not being included.

https://ruby-doc.org/core-2.5.1/Dir.html

Note, this will not match Unix-like hidden files (dotfiles). In order to include those in the match results, you must use the File::FNM_DOTMATCH flag or something like "{,.}".

BEFORE

$ omnibus new omnibus-test
      create  omnibus-test/Gemfile
      create  omnibus-test/gitignore
      create  omnibus-test/README.md
      create  omnibus-test/omnibus.rb
      create  omnibus-test/config/projects/openssl-fips.rb
      create  omnibus-test/config/software/openssl-fips-zlib.rb
Could not find ".kitchen.local.yml.erb" in any of your source paths. Your current source paths are:
/Users/iamjohnnym/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/omnibus-6.0.1/lib/omnibus/generator_files

AFTER

$ omnibus new test
      create  omnibus-test/Gemfile
      create  omnibus-test/.gitignore
      create  omnibus-test/README.md
      create  omnibus-test/omnibus.rb
      create  omnibus-test/config/projects/test.rb
      create  omnibus-test/config/software/test-zlib.rb
      create  omnibus-test/.kitchen.local.yml
      create  omnibus-test/.kitchen.yml
      create  omnibus-test/Berksfile
      create  omnibus-test/package-scripts/test/preinst
       chmod  omnibus-test/package-scripts/test/preinst
      create  omnibus-test/package-scripts/test/prerm
       chmod  omnibus-test/package-scripts/test/prerm
      create  omnibus-test/package-scripts/test/postinst
       chmod  omnibus-test/package-scripts/test/postinst
      create  omnibus-test/package-scripts/test/postrm
       chmod  omnibus-test/package-scripts/test/postrm

Maintainers

Please ensure that you check for:

  • If this change impacts git cache validity, it bumps the git cache
    serial number
  • If this change impacts compatibility with omnibus-software, the
    corresponding change is reviewed and there is a release plan
  • If this change impacts compatibility with the omnibus cookbook, the
    corresponding change is reviewed and there is a release plan

@iamjohnnym iamjohnnym requested a review from a team as a code owner January 7, 2019 21:52
@lamont-granquist
Copy link
Contributor

@iamjohnnym you should amend your commit to include a DCO sign off for us to accept this https://github.com/chef/chef/blob/master/CONTRIBUTING.md#developer-certification-of-origin-dco

(although if pressed i'd probably consider this a blatantly obvious fix)

travis seems incredibly angry but for what must be unrelated reasons...

@iamjohnnym
Copy link
Contributor Author

shooting from the hip but could the travis failures be related to bundler >= 2.0? I haven't personally had issues but I know its been a thing.

@iamjohnnym
Copy link
Contributor Author

iamjohnnym commented Jan 7, 2019

So, I found this to be a thing now that its copying the Gemfile over:

https://github.com/chef/omnibus/blob/master/lib/omnibus/generator_files/Gemfile.erb#L19

$ bundle install --binstubs

[!] There was an error parsing `Gemfile`: syntax error, unexpected tSTRING_BEG, expecting keyword_do or '{' or '(' -   gem 'kitchen-vagrant',
      ^
/Users/iamjohnnym/cookbooks/openvpn/omnibus-openssl-fips/Gemfile:20: syntax error, unexpected ',', expecting keyword_end
  gem 'kitchen-vagrant',
                       ^. Bundler cannot continue.

 #  from /Users/iamjohnnym/cookbooks/openvpn/omnibus-openssl-fips/Gemfile:20
 #  -------------------------------------------
 #    gem 'test-kitchen',
 >    gem 'kitchen-vagrant',
 #  end
 #  -------------------------------------------

Should I remove the commas and commit to this PR?

@marcparadise marcparadise added the Status: Waiting on Contributor A pull request that has unresolved requested actions from the author. label Feb 1, 2019
@marcparadise marcparadise removed the Status: Waiting on Contributor A pull request that has unresolved requested actions from the author. label Apr 1, 2019
@tas50
Copy link
Contributor

tas50 commented Apr 1, 2019

@iamjohnnym since we only wan't to capture the kitchen files, but not .DS_Store, .., and . entries we can probably use:

  gem.files = %w{ LICENSE README.md Rakefile Gemfile } + Dir.glob("*.gemspec") + Dir.glob("{bin,lib,resources,spec}/**/{*,.kitchen*}")

@pkleanthous-zz
Copy link

hey guys,

Any news on that?

@robbkidd
Copy link

robbkidd commented Apr 8, 2019

I've confirmed the related issue and run this fix locally to see that the new Dir includes hidden files.

I recommend we resolve whatever errors we need to, merge, and release.

@robbkidd
Copy link

robbkidd commented Apr 8, 2019

Force-pushed a rebase to get this branch up-to-date with master. Let's see what Travis says this time!

@iamjohnnym
Copy link
Contributor Author

Awesome. Sorry all for the MIA, been out since @tas50 updated. Is there anything you need from me?

@tas50
Copy link
Contributor

tas50 commented Apr 8, 2019

Update to the bit code I mentioned above and I think we're good to merge assuming others agree with that @robbkidd @lamont-granquist

@robbkidd
Copy link

robbkidd commented Apr 8, 2019

We can go with this change which may inadvertently include other dot-files when publishing this gem.

We can restrict the inclusion of dot-files to the ones starting with .kitchen like @tas50 suggests.

Or we can rename the two kitchen files (and update the code) to remove the dot from the filenames now that Test-Kitchen supports (prefers, even) a not-dotted kitchen.yml.

@iamjohnnym
Copy link
Contributor Author

Excellent. Give me a bit of time to implement @tas50 suggestion and test it locally.

cause arbitrary files from being picked up, resulting
in additional noise.  Nobody wants that.

@tas50 had a solid recommendation of being a bit
more explicit in what we want to copy across.   We
now explicitly request `.kitchen*` which will
copy the required kitchen files.

Signed-off-by: John Martin <[email protected]>
@iamjohnnym
Copy link
Contributor Author

Changes are pushed. I looked at writing some tests but I don't even know what that might look like for this specific issue. Open to suggestions if you feel its necessary.

Copy link

@robbkidd robbkidd left a comment

Choose a reason for hiding this comment

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

Quick test on the console:

[1] pry(main)> Dir.glob("{bin,lib,resources,spec}/**/{*,.kitchen*}").include? 'lib/omnibus/generator_files/.kitchen.local.yml.erb'
=> true

:shipit: 🚀🍰

tenor-201482594

@tas50 tas50 merged commit 17e7b39 into chef:master Apr 8, 2019
@tas50
Copy link
Contributor

tas50 commented Apr 8, 2019

Thanks @iamjohnnym

@iamjohnnym iamjohnnym deleted the bug/kitchen-files-missing branch April 8, 2019 20:54
@iamjohnnym
Copy link
Contributor Author

Anytime. Thanks for taking the time to come around to this. I had completely forgotten that I had opened it!

Much appreciated all, until next time.

@robbkidd
Copy link

robbkidd commented Apr 9, 2019

Thank you, @iamjohnnym, for submitting the fix! And thanks to @pkleanthous for bring it back to our attention when seeing the error again.

@pkleanthous-zz
Copy link

Hi @robbkidd

My pleasure to help the chef's community.

@robbkidd
Copy link

Fixed in omnibus 6.0.24 (which was also finally published to RubyGems).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants