Eliminate missing constant errors for LWRP class #5000

PrajaktaPurohit · 2016-06-06T19:48:02Z

http://manhattan.ci.chef.co/job/chef-trigger-ad_hoc/28/downstreambuildview/

➜  test-custom-resource-bug kitchen converge original-chef-12-9-41
-----> Starting Kitchen (v1.7.3)
-----> Creating <original-chef-12-9-41>...
       Bringing machine 'default' up with 'virtualbox' provider...
       ==> default: Importing base box 'bento/ubuntu-14.04'...
       ==> default: Matching MAC address for NAT networking...
       ==> default: Checking if box 'bento/ubuntu-14.04' is up to date...
       ==> default: A newer version of the box 'bento/ubuntu-14.04' is available! You currently
       ==> default: have version '2.2.5'. The latest is version '2.2.7'. Run
       ==> default: `vagrant box update` to update.
       ==> default: Setting the name of the VM: kitchen-test-custom-resource-bug-original-chef-12-9-41_default_1465241273190_44789
       ==> default: Fixed port collision for 22 => 2222. Now on port 2202.
       ==> default: Clearing any previously set network interfaces...
       ==> default: Preparing network interfaces based on configuration...
       ==> default: Clearing any previously set network interfaces...
       ==> default: Preparing network interfaces based on configuration...
           default: Adapter 1: nat
       ==> default: Forwarding ports...
           default: 22 => 2202 (adapter 1)
       ==> default: Booting VM...
       ==> default: Waiting for machine to boot. This may take a few minutes...
           default: SSH address: 127.0.0.1:2202
           default: SSH username: vagrant
           default: SSH auth method: private key
           default: Warning: Remote connection disconnect. Retrying...
           default: Warning: Remote connection disconnect. Retrying...
           default:
           default: Vagrant insecure key detected. Vagrant will automatically replace
           default: this with a newly generated keypair for better security.
           default:
           default: Inserting generated public key within guest...
           default: Removing insecure key from the guest if it's present...
           default: Key inserted! Disconnecting and reconnecting using new SSH key...
       ==> default: Machine booted and ready!
       ==> default: Checking for guest additions in VM...
       ==> default: Setting hostname...
       ==> default: Machine not provisioned because `--no-provision` is specified.
       [SSH] Established
       Vagrant instance <original-chef-12-9-41> created.
       Finished creating <original-chef-12-9-41> (0m38.05s).
-----> Converging <original-chef-12-9-41>...
       Preparing files for transfer
       Preparing dna.json
       Resolving cookbook dependencies with Berkshelf 4.3.3...
       Removing non-cookbook files before transfer
       Preparing validation.pem
       Preparing client.rb
-----> Installing Chef Omnibus (install only if missing)
       Downloading https://omnitruck.chef.io/current/install.sh to file /tmp/install.sh
       Trying wget...
       Download complete.
       ubuntu 14.04 x86_64
       Getting information for chef current  for ubuntu...
       downloading https://omnitruck.chef.io/current/chef/metadata?v=&p=ubuntu&pv=14.04&m=x86_64
         to file /tmp/install.sh.1376/metadata.txt
       trying wget...
       sha1     bc4a6642d6093b54de856da31f0651a56b0fdc8e
       sha256   f1cf5d0f6dd12d2d2296ec6d8dbb16363f8541f5c15298cafa70e65ff2b5a22f
       url      https://packages.chef.io/current/ubuntu/14.04/chef_12.11.18-1_amd64.deb
       version  12.11.18
       downloaded metadata file looks valid...
       downloading https://packages.chef.io/current/ubuntu/14.04/chef_12.11.18-1_amd64.deb
         to file /tmp/install.sh.1376/chef_12.11.18-1_amd64.deb
       trying wget...
       Comparing checksum with sha256sum...

       WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING

       You are installing an omnibus package without a version pin.  If you are installing
       on production servers via an automated process this is DANGEROUS and you will
       be upgraded without warning on new releases, even to new major releases.
       Letting the version float is only appropriate in desktop, test, development or
       CI/CD environments.

       WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING

       Installing chef
       installing with dpkg...
       Selecting previously unselected package chef.
(Reading database ... 38789 files and directories currently installed.)
       Preparing to unpack .../chef_12.11.18-1_amd64.deb ...
       Unpacking chef (12.11.18-1) ...
       Setting up chef (12.11.18-1) ...
       Thank you for installing Chef!
       Transferring files to <original-chef-12-9-41>
       Successfully installed appbundler-0.9.0
       Parsing documentation for appbundler-0.9.0
       Installing ri documentation for appbundler-0.9.0
       Done installing documentation for appbundler after 0 seconds
Fetching: appbundle-updater-0.2.12.gem (100%)
       Successfully installed appbundle-updater-0.2.12
       Parsing documentation for appbundle-updater-0.2.12
       Installing ri documentation for appbundle-updater-0.2.12
       Done installing documentation for appbundle-updater after 0 seconds
       2 gems installed
-----> Cleaning chef checkout
-----> Creating /opt/chef/embedded/apps directory
-----> Installing Packages
           running: apt-get -y update
           running: apt-get -q -y install build-essential git
-----> Extracting chef from https://github.com/chef/chef/archive/praj/FLOW-927/s3cmd.tar.gz
       Unkown tar entry: pax_global_header type: g.
-----> Installing dependencies
           running: /opt/chef/embedded/bin/ruby /opt/chef/embedded/bin/bundle install --without server docgen test development
-----> Installing gem
           running: /opt/chef/embedded/bin/ruby /opt/chef/embedded/bin/bundle exec /opt/chef/embedded/bin/rake install
-----> Updating appbundler binstubs for chef
           running: /opt/chef/embedded/bin/ruby /opt/chef/embedded/bin/appbundler /opt/chef/embedded/apps/chef /opt/chef/bin
-----> Finished!
       Starting Chef Client, version 12.11.18
       Creating a new client identity for original-chef-12-9-41 using the validator key.
       resolving cookbooks for run list: ["s3"]
       Synchronizing Cookbooks:
         - s3 (0.1.0)
       Installing Cookbook Gems:
       Compiling Cookbooks...
       AptRepository
       AptUpdate
       Batch
       Breakpoint
       ConvergeActions
       CookbookFile
       Cron
       DEPRECATED_OPTIONS
       Deploy
       DeprecatedLWRPClass
       Directory
       DscResource
       DscScript
       Env
       ErlCall
       Execute
       File
       Git
       Group
       HttpRequest
       Ifconfig
       InlineResources
       LWRPBase
       Launchd
       Link
       Log
       Mdadm
       Mount
       Noop
       Ohai
       OsxProfile
       Package
       PlatformDependentValue
       PlatformFamilyDependentValue
       PowershellScript
       Reboot
       RegistryKey
       RemoteDirectory
       RemoteFile
       ResourceRequirements
       Route
       RubyBlock
       Script
       Service
       Subversion
       SystemdUnit
       Template
       TemplateFinder
       User
       WhyrunSafeRubyBlock
       WindowsScript
       Converging 1 resources
       Recipe: s3::default
         * deploy_revision[/root/lessons] action nothing (skipped due to action :nothing)

       Running handlers:
       Running handlers complete

       Deprecated features used!
         Using an LWRP provider by its name (S3Scm) directly is no longer supported in Chef 12 and will be removed.  Use Chef::ProviderResolver.new(node, resource, action) instead. at 1 location:
           - /tmp/kitchen/cache/cookbooks/s3/recipes/default.rb:4:in `block in from_file'

       Chef Client finished, 0/1 resources updated in 01 seconds
       Finished converging <original-chef-12-9-41> (1m44.31s).
-----> Kitchen is finished. (2m23.02s)
➜  test-custom-resource-bug

coderanger · 2016-06-06T20:08:18Z

That was already a class method I think, so this is probably creating a global. If that's the goal (and you'll need a hell of a justification if thats the case) it should be $deprecated_constants to make that clear.

mwrock · 2016-06-07T03:07:43Z

I think this is a viable fix @coderanger. This PR aims to fix a backwards compatibility issue when using custom providers written under chef ( < 12.4) and running them under version 12.4 and greater. Lets say you have a scm.rb in your mycookbook provider folder and you have a recipe using this:

deploy_revision "/blah/blah" do
  scm_provider "mycookbook_scm"
  action :nothing
end

The problem today is that the @deprecated_constants hash is an instance variable (defined in a module) created and fed from an instance of the base chef::Provider but it is accessed from a derrived provider class Chef::Provider::Deploy and therefore it is a completely different hash.

So using a class variable ensures that the variable fed the deprecated constants and later accessed is the same object. Using $deprecated_constants would be a true global and more evil than @@deprecated_constants

mwrock · 2016-06-07T03:08:52Z

@PrajaktaPurohit I think if you rebase against master you will get the fixes to the dsc_script merged earlier today and perhaps turn this green.

coderanger · 2016-06-07T03:48:17Z

@mwrock My issue is that the module gets used via extend instead of include, I thought the semantics of that make it "class level" at Class.

mwrock · 2016-06-07T04:03:56Z

gotcha @coderanger. I think technically @deprecated_constants is class level like you say but the variable is written to as a class variable of Chef::Provider and then accessed from the class Chef::Provider::Deploy and therefore separate classes.

coderanger · 2016-06-07T04:07:22Z

Right, I think the change we want is to call Provider.deprecated_constants instead of self.deprecated_constants :)

mwrock · 2016-06-07T04:25:26Z

perhaps. currently the @deprecated_constants variable is wrapped via a private deprecated_constants method. We'd have to make that public and have the const_missing reference Provider.deprecated_constants which seems odd to do inside of the module that defines deprecated_constants.

Note this is pushing the envelope of my familiarity with ruby internals and usage patterns which is a super awesome learning experience but excuse me as I spew forth words that come from shallow knowledge.

coderanger · 2016-06-07T05:17:44Z

This (CoreClass.accessor) is a pretty common pattern, we do it in a lot of other places for registration stuffs. There is almost no time when you actually want a @@classvar, they are super error prone :)

coderanger · 2016-06-07T05:20:15Z

Ping @lamont-granquist @jkeiser to check that I'm not totally mis-remembering everything, which is never impossible.

mwrock · 2016-06-07T23:34:16Z

I think another rebase should green-up travis now @PrajaktaPurohit

mwrock · 2016-06-08T20:44:50Z

So the thing that feels weird to me now is the Chef::Provider.deprecated_constants inside the module code. This may be largely aesthetic but having this code exist as a module and then coupling it to the class seems like a bad move. If the choice is between that and the use of @@deprecated_constants I lean toward @@deprecated_constants.

Perhaps another route would be to have the deprecated_constant related methods pulled out of the module alltogether and placed directly in the class. This mirrors what is done in resource.rb. I have not tried this or scrutinized the nuances but if that "works" I think it may be the most straight forward path.

Thoughts @coderanger ?

coderanger · 2016-06-08T20:55:47Z

@mwrock We've gone pretty far in removing @@classvars from the codebase. Adding more seems like moving in the wrong direction given there is no actual need for it here and it's confusing enough that multiple Ruby devs are unsure of the precise semantics when combined with extend :)

mwrock · 2016-06-08T20:59:09Z

👍 on the recent change to whack the module which gets rid of the @@classvar and makes resource.rb and provider.rb follow a similar pattern which is less confusing (at least to me).

coderanger · 2016-06-08T21:05:55Z

Those new methods need very clear comments on them so we know what to remove in Chef 13, that's why they were in a module in the first place. 👎

mwrock · 2016-06-08T21:09:04Z

If we do put them in a module lets do it on both sides.

So maybe I'm just being overly anal on the use of the class from inside the module where is is used. Anyone else? I don't want to over think this.

coderanger · 2016-06-08T21:10:57Z

The reason for the module is to make life easier in Chef 13, so we can easily remove the code. @jkeiser has used that pattern in a lot of places now.

mwrock · 2016-06-08T21:12:51Z

that sounds reasonable and I'd assume we'd want to the removal on both sides.

mwrock · 2016-06-09T16:42:55Z

@PrajaktaPurohit put the module back and replicated in resource.rb as well. I'm still 👍

PrajaktaPurohit · 2016-06-09T17:00:13Z

@mwrock already done!

mwrock · 2016-06-09T17:04:25Z

oh sorry @PrajaktaPurohit , that wasn't intended to sound like a command(yuck) but simply restating what you had done. Thanks so much!

coderanger · 2016-06-09T19:12:40Z

lib/chef/provider.rb

      def deprecated_constants
+        raise "Deprecated constants should be called only on Chef::Provider" unless self == Chef::Provider


This needs an exception type. We should probably enable a RuboCop check for this :)

coderanger · 2016-06-09T19:14:10Z

One minor note about exception types if we want to care about this, if we don't do it now it will probably get fixed later when we enable that cop :) 👍 🚢

PrajaktaPurohit force-pushed the praj/FLOW-927/s3cmd branch 3 times, most recently from 0760143 to e5bcbaa Compare June 7, 2016 17:41

PrajaktaPurohit force-pushed the praj/FLOW-927/s3cmd branch from e5bcbaa to 16b0014 Compare June 8, 2016 00:06

PrajaktaPurohit changed the title ~~no more missing constants for LWRP class~~ WIP - no more missing constants for LWRP class Jun 8, 2016

PrajaktaPurohit force-pushed the praj/FLOW-927/s3cmd branch 2 times, most recently from 43d9d0b to 24a201c Compare June 8, 2016 19:19

PrajaktaPurohit changed the title ~~WIP - no more missing constants for LWRP class~~ no more missing constants for LWRP class Jun 8, 2016

PrajaktaPurohit force-pushed the praj/FLOW-927/s3cmd branch from 24a201c to fd87dc1 Compare June 8, 2016 20:55

no more missing constants for LWRP class

7c38923

PrajaktaPurohit force-pushed the praj/FLOW-927/s3cmd branch from fd87dc1 to 7c38923 Compare June 9, 2016 13:04

coderanger reviewed Jun 9, 2016
View reviewed changes

PrajaktaPurohit merged commit 7c6e771 into master Jun 10, 2016

PrajaktaPurohit deleted the praj/FLOW-927/s3cmd branch June 10, 2016 17:18

mwrock added the Bug label Jul 1, 2016

mwrock changed the title ~~no more missing constants for LWRP class~~ Eliminate missing constant errors for LWRP class Jul 1, 2016

thommay added Type: Bug Does not work as expected. and removed Bug labels Jan 25, 2017

chef locked and limited conversation to collaborators Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate missing constant errors for LWRP class #5000

Eliminate missing constant errors for LWRP class #5000

PrajaktaPurohit commented Jun 6, 2016

coderanger commented Jun 6, 2016

mwrock commented Jun 7, 2016

mwrock commented Jun 7, 2016

coderanger commented Jun 7, 2016

mwrock commented Jun 7, 2016

coderanger commented Jun 7, 2016

mwrock commented Jun 7, 2016

coderanger commented Jun 7, 2016

coderanger commented Jun 7, 2016

mwrock commented Jun 7, 2016

mwrock commented Jun 8, 2016

coderanger commented Jun 8, 2016

mwrock commented Jun 8, 2016

coderanger commented Jun 8, 2016

mwrock commented Jun 8, 2016

coderanger commented Jun 8, 2016

mwrock commented Jun 8, 2016

mwrock commented Jun 9, 2016

PrajaktaPurohit commented Jun 9, 2016

mwrock commented Jun 9, 2016

coderanger Jun 9, 2016

coderanger commented Jun 9, 2016

		def deprecated_constants
		raise "Deprecated constants should be called only on Chef::Provider" unless self == Chef::Provider

Eliminate missing constant errors for LWRP class #5000

Eliminate missing constant errors for LWRP class #5000

Conversation

PrajaktaPurohit commented Jun 6, 2016

coderanger commented Jun 6, 2016

mwrock commented Jun 7, 2016

mwrock commented Jun 7, 2016

coderanger commented Jun 7, 2016

mwrock commented Jun 7, 2016

coderanger commented Jun 7, 2016

mwrock commented Jun 7, 2016

coderanger commented Jun 7, 2016

coderanger commented Jun 7, 2016

mwrock commented Jun 7, 2016

mwrock commented Jun 8, 2016

coderanger commented Jun 8, 2016

mwrock commented Jun 8, 2016

coderanger commented Jun 8, 2016

mwrock commented Jun 8, 2016

coderanger commented Jun 8, 2016

mwrock commented Jun 8, 2016

mwrock commented Jun 9, 2016

PrajaktaPurohit commented Jun 9, 2016

mwrock commented Jun 9, 2016

coderanger Jun 9, 2016

Choose a reason for hiding this comment

coderanger commented Jun 9, 2016