Move parameter and property logic to separate classes #140

bpietraga · 2018-11-26T08:32:25Z

In this proposal the PR the changes were extraction of parameter logic to:

Puppet::ResourceApi::Property
Puppet::ResourceApi::ReadOnlyParameter
Puppet::ResourceApi::Parameter

and movement of value defaults and aliases logic to

Puppet::ResourceApi::ValueCreator

Commits are in smaller chunks and can be changed / squashed.
@DavidS if you have any changes, suggestions please let me know. I can adjust the code.

puppetlabs-jenkins · 2018-11-26T08:32:26Z

Can one of the admins verify this patch?

DavidS · 2018-11-26T08:47:43Z

I'll have 👀 on this later today.

DavidS

Hi @bpietraga ,

here's the first set of review comments. It's great to see the progress to date, and there's still a bunch of possible improvements. If you have any questions either myself or @da-ar are here for you.

For now I've focused on API design and library code.

Cheers, DavidS

lib/puppet/resource_api.rb

lib/puppet/resource_api/read_only_parameter.rb

lib/puppet/resource_api.rb

lib/puppet/resource_api/parameter.rb

lib/puppet/resource_api/value_creator.rb

bpietraga · 2018-11-26T15:26:50Z

@DavidS Thank you for the insight in this PR! I will work on it to adjust to suggestions. 🙂

bpietraga · 2018-11-27T17:04:31Z

I've included all suggestions from the comments, excluding the movement to ValueCreator from resource_api.rb to Parameter and Property classes. I will do it tomorrow 🙂The old puppet specs are now ok,.

lib/puppet/resource_api/parameter.rb

DavidS · 2018-11-28T14:25:42Z

I've added a small oversight from my initial review, otherwise 👍 , shaping up nicely. When you're satisfied with this iteration, I'll do a pass over the tests. although, from looking at codecov, they can't be that bad :-D

bpietraga · 2018-11-28T21:31:23Z

I wasn't sure about the attr_reader but this looks much cleaner. Thanks!
Also I've found that I've added unnecessary munge to Puppet::ResourceApi::ReadOnlyParameter as its already inheriting it from Puppet::ResourceApi::Parameter.

Also I've noticed weird behaviour when calling the methods from Puppet::ResourceApi::ValueCreator. defaultto and aliases are not working correctly when called from the initialize in resource. i.ex. Puppet::ResourceApi::Parameter. I'm afraid that debugging why the newvalue called on correct class is not working might take a while.

Current setup works ok with call to Puppet::ResourceApi::ValueCreator from the Puppet::ResourceApi.

If code is looks ok i would like move the calls to ValueCreator from the Puppet::ResourceApi to parameter and properties in next PR. I should be just different place to call the module.

If there is anything to change please write me 🙂

bpietraga · 2018-11-29T10:39:46Z

In last commit I've moved the call_to_provider(_value) from Puppet::ResourceApi::ValueCreator method to Puppet::ResourceApi::Property.

In original logic the @options.key?(:default) is duplicated. And the upper confitional covers the needed code flow.

Add Puppet::Util to paramater and property for support to older Puppet versions. Change the ValueCreator class to module. Provide test changes. Fix typos.

bpietraga · 2018-11-29T17:36:10Z

I've updated tests based on suggestions and also renamed commits to include (maint) to keep naming consistent. Thank you for the help in preparing this PR! 🙂

DavidS

Hi @bpietraga ,

apologies for another incomplete review.

Here are two items you can work on until I get around to the rest on Monday.

Regards, DavidS

lib/puppet/resource_api/property.rb

spec/puppet/resource_api/property_spec.rb

spec/puppet/resource_api/read_only_parameter_spec.rb

spec/puppet/resource_api/property_spec.rb

lib/puppet/resource_api/property.rb

spec/puppet/resource_api/value_creator_spec.rb

Rename resource_class to attribute_class to avoid confusion in Puppet::ResourceApi::ValueCreator. Update `value_creator_spec.rb` to check methods call arguments.

Update ruby-puppet-resource_api to 1.8.12. ## [1.8.7](https://github.com/puppetlabs/puppet-resource_api/tree/1.8.7) (2019-09-11) [Full Changelog](puppetlabs/puppet-resource_api@1.8.6...1.8.7) **Fixed bugs:** - \(FM-8092\) Fix caching scope of transport schemas [\#200](puppetlabs/puppet-resource_api#200) ([DavidS](https://github.com/DavidS)) **Merged pull requests:** - \(FM-8485\) - Addition of CODEOWNERS file [\#203](puppetlabs/puppet-resource_api#203) ([david22swan](https://github.com/david22swan)) - \(MODULES-9258\) Improve referencing and add summary [\#199](puppetlabs/puppet-resource_api#199) ([MaxMagill](https://github.com/MaxMagill)) - \(maint\) Pin both Jruby cells to use `dist: trusty` [\#197](puppetlabs/puppet-resource_api#197) ([da-ar](https://github.com/da-ar)) ## [v1.8.6](https://github.com/puppetlabs/puppet-resource_api/tree/v1.8.6) (2019-07-01) [Full Changelog](puppetlabs/puppet-resource_api@1.8.5...v1.8.6) **Implemented enhancements:** - \(SERVER-2470\) list\_all\_transports implementation for puppetserver [\#187](puppetlabs/puppet-resource_api#187) ([DavidS](https://github.com/DavidS)) **Fixed bugs:** - \(MODULES-9428\) make the composite namevar implementation usable [\#174](puppetlabs/puppet-resource_api#174) ([DavidS](https://github.com/DavidS)) **Merged pull requests:** - Merge 1.6.x [\#194](puppetlabs/puppet-resource_api#194) ([da-ar](https://github.com/da-ar)) - \(maint\) test fixes [\#193](puppetlabs/puppet-resource_api#193) ([DavidS](https://github.com/DavidS)) - \(packaging\) Revert to version '1.8.5' \[no-promote\] [\#192](puppetlabs/puppet-resource_api#192) ([gimmyxd](https://github.com/gimmyxd)) - \(packaging\) Bump to version '1.9.0' \[no-promote\] [\#191](puppetlabs/puppet-resource_api#191) ([gimmyxd](https://github.com/gimmyxd)) ## [1.8.5](https://github.com/puppetlabs/puppet-resource_api/tree/1.8.5) (2019-06-24) [Full Changelog](puppetlabs/puppet-resource_api@1.8.4...1.8.5) **Fixed bugs:** - \(maint\) Mergeup 1.6.x: FM-7839, desc/docs cleanup [\#186](puppetlabs/puppet-resource_api#186) ([DavidS](https://github.com/DavidS)) **Merged pull requests:** - \(maint\) reduce debug noise caused by `feature?` [\#189](puppetlabs/puppet-resource_api#189) ([da-ar](https://github.com/da-ar)) - \(FM-8265\) Merge branch '1.6.x' into master [\#188](puppetlabs/puppet-resource_api#188) ([da-ar](https://github.com/da-ar)) - \(maint\) test fixes [\#185](puppetlabs/puppet-resource_api#185) ([DavidS](https://github.com/DavidS)) - \(maint\) make test order really random [\#175](puppetlabs/puppet-resource_api#175) ([DavidS](https://github.com/DavidS)) - \(packaging\) Update reported version to 1.8.4 \[no-promote\] [\#171](puppetlabs/puppet-resource_api#171) ([gimmyxd](https://github.com/gimmyxd)) ## [1.8.4](https://github.com/puppetlabs/puppet-resource_api/tree/1.8.4) (2019-06-12) [Full Changelog](puppetlabs/puppet-resource_api@1.8.3...1.8.4) **Implemented enhancements:** - \(FM-7839\) Implement `to\_json` method for ResourceShim [\#168](puppetlabs/puppet-resource_api#168) ([da-ar](https://github.com/da-ar)) **Fixed bugs:** - \(maint\) backport minor fixes from master to 1.6.x [\#184](puppetlabs/puppet-resource_api#184) ([DavidS](https://github.com/DavidS)) - \(PUP-9747\) Relax validation for bolt [\#182](puppetlabs/puppet-resource_api#182) ([DavidS](https://github.com/DavidS)) - \(maint\) Add to\_hash function to resourceShim for compatibility [\#180](puppetlabs/puppet-resource_api#180) ([da-ar](https://github.com/da-ar)) - \(maint\) implement `desc`/`docs` fallback [\#177](puppetlabs/puppet-resource_api#177) ([DavidS](https://github.com/DavidS)) **Closed issues:** - ResourceShim should respond to to\_hash [\#179](puppetlabs/puppet-resource_api#179) **Merged pull requests:** - \(maint\) Merge 1.6.x to master [\#183](puppetlabs/puppet-resource_api#183) ([mihaibuzgau](https://github.com/mihaibuzgau)) - \(maint\) Fixup Gemfile for JRuby 1.7 installs [\#173](puppetlabs/puppet-resource_api#173) ([da-ar](https://github.com/da-ar)) - \(maint\) test cleanups [\#172](puppetlabs/puppet-resource_api#172) ([DavidS](https://github.com/DavidS)) ## [1.8.3](https://github.com/puppetlabs/puppet-resource_api/tree/1.8.3) (2019-04-12) [Full Changelog](puppetlabs/puppet-resource_api@1.8.2...1.8.3) **Fixed bugs:** - \(FM-7867\) Always throw when transport schema validation fails [\#169](puppetlabs/puppet-resource_api#169) ([da-ar](https://github.com/da-ar)) **Merged pull requests:** - \(PA-2496\) Bump version and remove v from version number [\#170](puppetlabs/puppet-resource_api#170) ([mihaibuzgau](https://github.com/mihaibuzgau)) ## [1.8.2](https://github.com/puppetlabs/puppet-resource_api/tree/1.8.2) (2019-04-10) [Full Changelog](puppetlabs/puppet-resource_api@v1.6.4...1.8.2) **Merged pull requests:** - \(packaging\) Update reported version to 1.8.2 \[no-promote\] [\#167](puppetlabs/puppet-resource_api#167) ([mihaibuzgau](https://github.com/mihaibuzgau)) ## [v1.6.4](https://github.com/puppetlabs/puppet-resource_api/tree/v1.6.4) (2019-03-25) [Full Changelog](puppetlabs/puppet-resource_api@v1.8.1...v1.6.4) **Merged pull requests:** - Add `implementations` to reserved bolt keywords [\#165](puppetlabs/puppet-resource_api#165) ([DavidS](https://github.com/DavidS)) - \(MAINT\) Bump version [\#164](puppetlabs/puppet-resource_api#164) ([sebastian-miclea](https://github.com/sebastian-miclea)) - Release prep for v1.8.1 [\#163](puppetlabs/puppet-resource_api#163) ([DavidS](https://github.com/DavidS)) # Changelog All significant changes to this repo will be summarized in this file. ## [v1.8.1](https://github.com/puppetlabs/puppet-resource_api/tree/v1.8.1) (2019-03-13) [Full Changelog](puppetlabs/puppet-resource_api@v1.8.0...v1.8.1) **Fixed bugs:** - \(maint\) Fixes sensitive transport values where absent keys are wrapped [\#161](puppetlabs/puppet-resource_api#161) ([da-ar](https://github.com/da-ar)) **Merged pull requests:** - 1.6.x mergeup [\#162](puppetlabs/puppet-resource_api#162) ([DavidS](https://github.com/DavidS)) - \(FM-7829\) Update README with transports examples [\#160](puppetlabs/puppet-resource_api#160) ([willmeek](https://github.com/willmeek)) - \(maint\) update release docs [\#159](puppetlabs/puppet-resource_api#159) ([DavidS](https://github.com/DavidS)) - Improve travis cells and testing [\#145](puppetlabs/puppet-resource_api#145) ([DavidS](https://github.com/DavidS)) ## [v1.8.0](https://github.com/puppetlabs/puppet-resource_api/tree/v1.8.0) (2019-02-26) [Full Changelog](puppetlabs/puppet-resource_api@v1.7.0...v1.8.0) **Implemented enhancements:** - \(FM-7695\) Transports - the remote content framework [\#157](puppetlabs/puppet-resource_api#157) ([DavidS](https://github.com/DavidS)) - \(FM-7698\) implement `sensitive:true` handling [\#156](puppetlabs/puppet-resource_api#156) ([da-ar](https://github.com/da-ar)) - \(PDK-1271\) Allow a transport to be wrapped and used like a device [\#155](puppetlabs/puppet-resource_api#155) ([da-ar](https://github.com/da-ar)) - \(FM-7701\) Support device providers when using Transport Wrapper [\#154](puppetlabs/puppet-resource_api#154) ([da-ar](https://github.com/da-ar)) - \(FM-7726\) implement `context.transport` to provide access [\#152](puppetlabs/puppet-resource_api#152) ([DavidS](https://github.com/DavidS)) - \(FM-7674\) Allow wrapping a Transport in a legacy Device [\#149](puppetlabs/puppet-resource_api#149) ([da-ar](https://github.com/da-ar)) - \(FM-7600\) Add Transport.connect method [\#148](puppetlabs/puppet-resource_api#148) ([da-ar](https://github.com/da-ar)) **Fixed bugs:** - \(FM-7690\) Fix transports cache to be environment aware [\#151](puppetlabs/puppet-resource_api#151) ([da-ar](https://github.com/da-ar)) **Merged pull requests:** - \(FM-7726\) cleanups for the transport [\#153](puppetlabs/puppet-resource_api#153) ([DavidS](https://github.com/DavidS)) - \(FM-7691,FM-7696\) refactoring definition handling in contexts [\#150](puppetlabs/puppet-resource_api#150) ([DavidS](https://github.com/DavidS)) ## [v1.7.0](https://github.com/puppetlabs/puppet-resource_api/tree/v1.7.0) (2019-01-07) [Full Changelog](puppetlabs/puppet-resource_api@v1.6.3...v1.7.0) **Implemented enhancements:** - \(maint\) Validate Type Schema [\#142](puppetlabs/puppet-resource_api#142) ([da-ar](https://github.com/da-ar)) **Merged pull requests:** - \(maint\) Bundler 2.0 dropped support for Ruby versions \< 2.2 [\#147](puppetlabs/puppet-resource_api#147) ([da-ar](https://github.com/da-ar)) - \(FM-7597\) RSAPI Transport register function [\#146](puppetlabs/puppet-resource_api#146) ([da-ar](https://github.com/da-ar)) - \(packaging\) Update version to 1.7.0 [\#144](puppetlabs/puppet-resource_api#144) ([branan](https://github.com/branan)) ## [v1.6.3](https://github.com/puppetlabs/puppet-resource_api/tree/v1.6.3) (2018-12-11) [Full Changelog](puppetlabs/puppet-resource_api@v1.6.2...v1.6.3) **Closed issues:** - Trying to understand stubbing in the examples [\#136](puppetlabs/puppet-resource_api#136) **Merged pull requests:** - \(packaging\) Update version to 1.6.3 [\#143](puppetlabs/puppet-resource_api#143) ([branan](https://github.com/branan)) - Move parameter and property logic to separate classes [\#140](puppetlabs/puppet-resource_api#140) ([bpietraga](https://github.com/bpietraga)) - \(maint\) Predeclare Puppet module before ResourceApi [\#139](puppetlabs/puppet-resource_api#139) ([caseywilliams](https://github.com/caseywilliams)) - \(maint\) minor fix to make data\_type\_handling change work [\#138](puppetlabs/puppet-resource_api#138) ([DavidS](https://github.com/DavidS)) - \(maint\) extract data type handling code [\#137](puppetlabs/puppet-resource_api#137) ([bpietraga](https://github.com/bpietraga)) - Release prep for v1.6.2 [\#135](puppetlabs/puppet-resource_api#135) ([DavidS](https://github.com/DavidS))

DavidS suggested changes Nov 26, 2018

View reviewed changes

DavidS reviewed Nov 28, 2018

View reviewed changes

lib/puppet/resource_api/parameter.rb Outdated Show resolved Hide resolved

Bernard Pietraga and others added 8 commits November 29, 2018 18:05

(maint) move parameter and property logic to classes

890184b

(maint) move value handling logic to ValueCreator

02867d1

(maint) remove duplicated :default key check in ValueCreator

ee55a77

In original logic the @options.key?(:default) is duplicated. And the upper confitional covers the needed code flow.

(maint) add ValueCreator docs and small restructure in ResourceApi

ad47c17

(maint) change parameter and properties class structure

b7c636d

Add Puppet::Util to paramater and property for support to older Puppet versions. Change the ValueCreator class to module. Provide test changes. Fix typos.

(maint) move the call_provider method to property

8ace289

(maint) update parameter tests

a0d1cbb

(maint) update property and read only parameter tests

430f162

(maint) Delete unnecessary requires in resource_api.rb

48957a8

DavidS suggested changes Nov 30, 2018

View reviewed changes

lib/puppet/resource_api/property.rb Outdated Show resolved Hide resolved

spec/puppet/resource_api/property_spec.rb Outdated Show resolved Hide resolved

bpietraga commented Dec 3, 2018

View reviewed changes

spec/puppet/resource_api/read_only_parameter_spec.rb Show resolved Hide resolved

bpietraga commented Dec 3, 2018

View reviewed changes

spec/puppet/resource_api/property_spec.rb Show resolved Hide resolved

(maint) Update resource tests

a96abac

bpietraga commented Dec 3, 2018

View reviewed changes

lib/puppet/resource_api/property.rb Show resolved Hide resolved

(maint) Add class check for property class #insync? overload

003726f

DavidS reviewed Dec 4, 2018

View reviewed changes

spec/puppet/resource_api/value_creator_spec.rb Outdated Show resolved Hide resolved

DavidS reviewed Dec 4, 2018

View reviewed changes

spec/puppet/resource_api/value_creator_spec.rb Outdated Show resolved Hide resolved

Bernard Pietraga added 3 commits December 4, 2018 15:58

(maint) Add rename in ValueCreator and update tests

f91bb12

Rename resource_class to attribute_class to avoid confusion in Puppet::ResourceApi::ValueCreator. Update `value_creator_spec.rb` to check methods call arguments.

(maint) Change the definition of #insync?

87d8b0f

(maint) Update :ensure property tests

83f99db

DavidS approved these changes Dec 6, 2018

View reviewed changes

DavidS merged commit 3099283 into puppetlabs:master Dec 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move parameter and property logic to separate classes #140

Move parameter and property logic to separate classes #140

bpietraga commented Nov 26, 2018

puppetlabs-jenkins commented Nov 26, 2018

DavidS commented Nov 26, 2018

DavidS left a comment

bpietraga commented Nov 26, 2018

bpietraga commented Nov 27, 2018

DavidS commented Nov 28, 2018

bpietraga commented Nov 28, 2018

bpietraga commented Nov 29, 2018

bpietraga commented Nov 29, 2018

DavidS left a comment

Move parameter and property logic to separate classes #140

Move parameter and property logic to separate classes #140

Conversation

bpietraga commented Nov 26, 2018

puppetlabs-jenkins commented Nov 26, 2018

DavidS commented Nov 26, 2018

DavidS left a comment

Choose a reason for hiding this comment

bpietraga commented Nov 26, 2018

bpietraga commented Nov 27, 2018

DavidS commented Nov 28, 2018

bpietraga commented Nov 28, 2018

bpietraga commented Nov 29, 2018

bpietraga commented Nov 29, 2018

DavidS left a comment

Choose a reason for hiding this comment