Closed tas50 closed 6 years ago
We probably don't need the Vagrantfile anymore. Kitchen should be handling this for us no?
Ping. It would be great to get this merged in and released
Hi!
We're stuck on merging a downstream change due to this being broken right now.
Any chance we can ship and merge this please?
Hi!
When I attempt to run the kitchen test suite for this cookbook (the default-ubuntu-1404
instance), the changes on this branch cause the tests to fail. I've included the output below - you can reproduce it by running kitchen test
locally.
================================================================================
Recipe Compile Error in /tmp/kitchen/cache/cookbooks/yum/resources/globalconfig.rb
================================================================================
ArgumentError
-------------
Cannot specify both default and name_property/name_attribute together on property path of resource yum_globalconfig
Cookbook Trace:
---------------
/tmp/kitchen/cache/cookbooks/yum/resources/globalconfig.rb:76:in `class_from_file'
Relevant File Content:
----------------------
/tmp/kitchen/cache/cookbooks/yum/resources/globalconfig.rb:
69: attribute :mdpolicy, :kind_of => String, :equal_to => %w(instant group:primary group:small group:main group:all), :default => nil
70: attribute :metadata_expire, :kind_of => String, :regex => [/^\d+$/, /^\d+[mhd]$/, /never/], :default => nil
71: attribute :mirrorlist_expire, :kind_of => String, :regex => /^\d+$/, :default => nil
72: attribute :multilib_policy, :kind_of => String, :equal_to => %w(all best), :default => nil
73: attribute :obsoletes, :kind_of => [TrueClass, FalseClass], :default => nil
74: attribute :overwrite_groups, :kind_of => [TrueClass, FalseClass], :default => nil
75: attribute :password, :kind_of => String, :regex => /.*/, :default => nil
76>> attribute :path, :kind_of => String, :regex => /.*/, :default => nil, :name_attribute => true
77: attribute :persistdir, :kind_of => String, :regex => /.*/, :default => nil
78: attribute :pluginconfpath, :kind_of => String, :regex => /.*/, :default => nil
79: attribute :pluginpath, :kind_of => String, :regex => /.*/, :default => nil
80: attribute :plugins, :kind_of => [TrueClass, FalseClass], :default => true
81: attribute :protected_multilib, :kind_of => [TrueClass, FalseClass], :default => nil
82: attribute :protected_packages, :kind_of => String, :regex => /.*/, :default => nil
83: attribute :proxy, :kind_of => String, :regex => /.*/, :default => nil
84: attribute :proxy_password, :kind_of => String, :regex => /.*/, :default => nil
85: attribute :proxy_username, :kind_of => String, :regex => /.*/, :default => nil
System Info:
------------
chef_version=13.1.31
platform=ubuntu
platform_version=14.04
ruby=ruby 2.4.1p111 (2017-03-22 revision 58053) [x86_64-linux]
program_name=chef-client worker: ppid=1226;start=07:14:23;
executable=/opt/chef/bin/chef-client
Hi @ice799 I'm not getting this errror on tas50's branch
Finished in 0.11124 seconds (files took 0.34092 seconds to load)
15 examples, 0 failures
Hi @damacus. The test output you've provided is not from the kitchen ci tests. Those are the chefspec/rspec tests. We also need the kitchen ci tests to pass before merging this. There's a few changes provided in this PR that unfortunately, I won't be able to merge. I'll try to leave a review for those changes, soon.
Nope , they are the inspec tests passing.
I can paste the full link if you'd like?
We're finding it easier in sous-chefs to use Travis automated testing to complete the testing for us automatically so everyone can see the output.
The Haproxy repo has a good bunch of Travis settings and dokken integrated testing
Hi @damacus, please see my previous reply here: https://github.com/computology/packagecloud-cookbook/pull/53#issuecomment-308029469.
Until kitchen test
passes I can not merge this PR. It does not currently pass. It is not run automatically with Travis. Some changes must also be made to this PR before it can be merged. I will do a review soon. Thanks!
See below for me checking out tas50's repo, switching to the fork, and running kitchen test.
So I'm not sure which branch you're testing on. Possibly the main branch in the package-cloud repo?
Hi @ice799 any update on this one please?
I'd like to see an update about this PR as well. I'm about to submit a separate one which is based off master to make this a Chef 12.6+ based cookbook.
@ice799 I updated the test recipe to specify the rubygems source with the install, which is required with Chef 13. I also added kitchen runs against Chef 12.1 and the latest release on centos 6/7 and ubuntu 14.04/16.04. Passing Travis tests now should give a pretty high degree of confidence. Let me know if there's anything else you need to get this merged/released?
Hi:
Some initial feedback:
ArgumentError
-------------
Cannot specify both default and name_property/name_attribute together on property path of resource yum_globalconfig
Which seems to be due to:
76>> attribute :path, :kind_of => String, :regex => /.*/, :default => nil, :name_attribute => true
The SHA on the tree with which I am testing is: 8a7b55cf831b00fe09e9954fcdb132a322410ebe1
.
I've added back Test Kitchen CentOS 5 / Ubuntu 12.04 testing. The yum error your getting is due to an old version of yum on your local system. You should be able to run berks update
to resolve that.
Here's the latest release: https://github.com/chef-cookbooks/yum/blob/master/resources/globalconfig.rb#L73
Hi @tas50. I've updated and re-run the tests. All tests pass for Ubuntu 12.04 - 16.04. Unfortuantely, the tests for CentOS 5.11 seem to fail with the following error for me:
---- Begin output of /usr/bin/python /opt/chef/embedded/lib/ruby/gems/2.4.0/gems/chef-13.2.20/lib/chef/provider/package/yum/yum-dump.py --options --installed-provides --yum-lock-timeout 30 ----
STDOUT: [option installonlypkgs] kernel kernel-bigmem kernel-enterprise kernel-smp kernel-modules kernel-debug kernel-unsupported kernel-source kernel-devel kernel-PAE kernel-PAE-debug
YumRepo Error: All mirror URLs are not using ftp, http[s] or file.
Eg. Invalid release/
removing mirrorlist with no valid mirrors: /var/cache/yum/base/mirrorlist.txt
STDERR: yum-dump Repository Error: Cannot find a valid baseurl for repo: base
---- End output of /usr/bin/python /opt/chef/embedded/lib/ruby/gems/2.4.0/gems/chef-13.2.20/lib/chef/provider/package/yum/yum-dump.py --options --installed-provides --yum-lock-timeout 30 ----
Ran /usr/bin/python /opt/chef/embedded/lib/ruby/gems/2.4.0/gems/chef-13.2.20/lib/chef/provider/package/yum/yum-dump.py --options --installed-provides --yum-lock-timeout 30 returned 1
I haven't had a chance to debug it yet, but wanted to update this thread.
It appears that the docker centos 5 box hasn't been updated since they archived the mirrors. I'll see if I can get that working locally
@ice799 Can you try running the centos 5 test again. I've updated the docker image for centos5 to fix the repos.
I've closed my PR in favor of this one as a small step forward to modernizing this cookbook. A better pattern down the road will be to separate each of the package providers into their own platform based provider (rpm, apt, etc) so the code isn't so complex in terms of platform detection.
Where are we with this change set?
@ice799 Any objections with this now? CentOS 5 testing in Travis is a no go due to networking issues in the centos-5 container. I have it working and passing for the RPM stuff in Vagrant. The Ruby stuff doesn't work since gem is so old on CentOS 5 that it doesn't support source so the entire resource doesn't work there. I'd update gem, but Ruby is so old there that it doesn't support the rubygems updater. Old and old.
bump. Is there any chance we can get this merged and cut a new version?
I emailed @ice799 to have him look at this PR again. If it can't be merged then we'll probably create our own internal implementation and eventually phase out this cookbook. Don't see the hold up here to hang on to one VERY old platform which is most likely not in use. Chef 14's release this April will start a timer for us phasing out this cookbook if even this PR cannot be merged in time. 😢
@martinisoft, please don't create own internal implementation and create open fork instead.
@tas50
The code as-is currently fails when I run kitchen test, but I was able to get the CentOS 5 tests working by making a few small changes. It seems that the newer version of chef broke gem_package
for CentOS 5 so I simply shell out to gem install
in the CentOS 5 case and everything works as expected.
I tried to push a commit to your branch on your repo (https://help.github.com/articles/committing-changes-to-a-pull-request-branch-created-from-a-fork/), but I think perhaps changes may not be allowed (https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/).
Here's a diff I created that should apply cleanly to your branch: https://gist.github.com/ice799/49bef10a9b5915d60f5cb8678589e05a
When I apply this diff and run kitchen test all tests succeed -- if you want to apply this diff and verify it works for you and push it up, I think we can probably merge this after that.
Sorry for the pain around CentOS 5 -- we have a considerable number of customers using CentOS 5 so we need to continue supporting it for now.
If CentOS 5 support is such a sticking point then I'd propose this PR mark a breaking version number to allow you to move forward. Fixes for that platform could be backported if needed to matching version numbers there.
This breaking version would have to be Chef 12.7+ and would break support for CentOS 5. Food for thought is that CentOS 5 isn't officially supported by Chef anymore so it's fair to break it here and move on as the Chef community has right now. Chef 12 support will also be EOL as of April 30th of this year.
This proposal work for you @ice799 ?
@martinisoft I'll merge once the tests pass. The tests do not currently pass when you run kitchen test on this branch because the CentOS 5 tests run and fail on this branch without my patch. My proposed patch to this branch fixes the tests and makes this mergeable.
@ice799 'Allow edits from maintainers.' is enabled so you should be able to continue to work on this PR.
@ice799 The PR is passing now
1 change per commit. This syncs up testing to Chef best practices and fixes warnings / failures in Chef 13