Development for new feature types and providers #18

ericzji · 2017-11-21T19:03:08Z

The new types and providers added:

f5_persistencecookie
f5_persistencedestaddr
f5_persistencehash
f5_persistencesourceaddr
f5_persistencessl
f5_persistenceuniversal
f5_profilehttp
f5_profileclientssl
f5_profileserverssl
f5_sslkey
f5_sslcertificate
f5_snat
f5_snatpool
f5_datagroup
f5_datagroupexternal

…development

alexjfisher · 2017-11-22T10:01:17Z

lib/puppet/provider/f5_persistencecookie/rest.rb

+    map = {
+      :'always-send'          => :alwaysSend,
+      :'cookie-encryption'    => :cookieEncryption,
+      :'cookie-name'          => :cookieName,


indentation

alexjfisher · 2017-11-22T10:01:25Z

lib/puppet/provider/f5_persistencecookie/rest.rb

+
+    message = strip_nil_values(message)
+    message = convert_underscores(message)
+    #message = gen_sflow(message)


Delete line

alexjfisher · 2017-11-22T10:10:46Z

lib/puppet/provider/f5_persistencehash/rest.rb

+      :'match-across-virtuals'       => :matchAcrossVirtuals,
+      :'hash-algorithm'              => :hashAlgorithm,
+      :'hash-offset'              => :hashOffset,
+      :'hash-length'              => :hashLength,


Indentation.

alexjfisher · 2017-11-22T10:16:10Z

lib/puppet/provider/f5_snat/rest.rb

+  end
+
+  def destroy
+    full_path_uri = resource[:name].gsub('/','~')


There's a lot of repetition. Could the full_path_uri code be moved to the parent provider and shared?

It's possible, but let's take it separately as it applies changes to ALL modules. @abrader

yeah, I guess. With each new type added, the duplicated code becomes more of an issue though.

@alexjfisher good point, discussed this with @ericzji and best suited as a PR of it's own. Will keep you posted.

alexjfisher · 2017-11-22T10:17:20Z

lib/puppet/provider/f5_snat/rest.rb

+    return [] if profiles.nil?
+
+    profiles.each do |profile|
+      full_path_uri = profile['fullPath'].gsub('/','~')


What's this used for?

@alexjfisher Not sure about the question, can you elaborate it?

It's a variable local to instances and is not used. If you were to run rubocop, it would say...

Useless assignment to variable - full_path_uri.

alexjfisher · 2017-11-22T10:18:52Z

lib/puppet/provider/f5_sslcertificate/rest.rb

+    message = object.to_hash
+
+    # Map for conversion in the message.
+    map = {


map isn't used?

Not used now, but leave it there for possible future use

I disagree. Leaving dead unused code everywhere is not helpful.

I have to agree with @alexjfisher here. If you need to reference the mapping model for there are plenty of providers where you can copy that code at the time of need @ericzji. Unused code in general should be removed. Furthermore we should get you running your code through rubocop.

alexjfisher · 2017-11-22T10:21:05Z

lib/puppet/provider/f5_sslkey/rest.rb

+  end
+
+  def exists?
+    @property_hash[:ensure] == :present


If you don't implement instances, @property_hash will never be set.

You are right. In this case, it does not need to. @property_hash[:ensure] not being set, will trigger "create". @abrader

on every single run...

@ericzji @alexjfisher absolutely agreed.

Modified " @property_hash[:ensure] == :present" to "false", and it's tested

alexjfisher · 2017-11-22T10:24:24Z

lib/puppet/type/f5_sslkey.rb

+  @doc = ' Import SSL Keys'
+
+  apply_to_device
+  ensurable


It looks like this type shouldn't use ensurable.

@ericzji let's review this one more closely today.

alexjfisher · 2017-11-22T10:28:06Z

lib/puppet/provider/f5_sslkey/rest.rb

+  def create
+    result = Puppet::Provider::F5.post("/mgmt/tm/sys/crypto/key", message(resource))
+    # We clear the hash here to stop flush from triggering.
+    @property_hash.clear


@property_hash isn't set, so doesn't need clearing.

I am leaving it to @abrader for comment

@ericzji let's review today

alexjfisher · 2017-11-22T10:34:36Z

lib/puppet/type/f5_persistencecookie.rb

+
+  newproperty(:method) do
+    desc "Fail Safe Action. Valid values are 'reboot' or 'restart-all'."
+    newvalues(:'insert', :'passive', 'rewrite')


Are these strings or symbols?

I agree with @alexjfisher in that you @ericzji should make all of these symbols (i.e., turn 'rewrite' into :rewrite)

alexjfisher · 2017-11-22T10:41:36Z

lib/puppet/type/f5_persistencecookie.rb

+  end
+
+  newproperty(:httponly, :parent => Puppet::Property::F5truthy) do
+    truthy_property("Valid values are 'enabled' or 'disabled'.")


Having looked at truthy_property and its use in other types, proper booleans true and false should also work. I think the string passed in should describe the property not just say Valid values are 'enabled' or 'disabled' each time.

only 'enabled' or 'disabled' allowed

No, the whole purpose of truthy_property is to allow true/false to be used. truthy_property munges boolean-like values into enabled/disabled.

@ericzji this is true. @alexjfisher is correct in that you should let the f5_truthy property accept lazy values of what is effectively true or false. This gives user flexibility which is helpful.

alexjfisher · 2017-11-22T10:43:18Z

lib/puppet/type/f5_persistencedestaddr.rb

+
+  newproperty(:hash_algorithm) do
+    desc "hash_algorithm."
+    newvalues(:default, :'carp')


Why the quotes around the symbol :carp?

it's string

@ericzji change 'carp' to :carp

alexjfisher · 2017-11-22T10:44:39Z

lib/puppet/type/f5_persistencehash.rb

+  end
+
+ newproperty(:hash_algorithm) do
+    desc "hash_algorithm."


If the property name is self explanatory, maybe just omit the desc.

Will consider, but leave it now

@ericzji we can review this one.

alexjfisher · 2017-11-22T10:46:02Z

lib/puppet/type/f5_profileclientssl.rb

+
+  newproperty(:proxy_ssl, :parent => Puppet::Property::F5truthy) do
+    desc "Valid values are 'enabled' or 'disabled'."
+    truthy_property('Fail Safe')


Is Fail Safe a good description?

@ericzji should get all description fields from iControl documentation since it is provided.

alexjfisher · 2017-11-22T10:46:09Z

lib/puppet/type/f5_profileclientssl.rb

+
+  newproperty(:proxy_ssl_passthrough, :parent => Puppet::Property::F5truthy) do
+    desc "Valid values are 'enabled' or 'disabled'."
+    truthy_property('Fail Safe')


@ericzji same as above.

alexjfisher · 2017-11-22T10:47:19Z

lib/puppet/type/f5_profileclientssl.rb

+
+  newproperty(:peer_cert_mode) do
+    desc "peer_cert_mode."
+    newvalues(:'ignore', :'require')


Quoting rubobop...

Do not use strings for word-like symbol literals

@ericzji please change 'ignore' and 'require' to :ignore and :require accordingly.

alexjfisher · 2017-11-22T10:54:43Z

lib/puppet/type/f5_profileserverssl.rb

+  ensurable
+
+  newparam(:name) do
+    def self.postinit


I've not come across postinit before. I'd like to understand better what this actually does. @hunner ?

alexjfisher · 2017-11-22T10:56:25Z

lib/puppet/type/f5_snat.rb

+require File.expand_path(File.join(File.dirname(__FILE__),'..','..','puppet/property/f5_description.rb'))
+require File.expand_path(File.join(File.dirname(__FILE__),'..','..','puppet/property/f5_health_monitors.rb'))
+require File.expand_path(File.join(File.dirname(__FILE__),'..','..','puppet/property/f5_ratio.rb'))
+require File.expand_path(File.join(File.dirname(__FILE__),'..','..','puppet/property/f5_state.rb'))


Are all these requires actually doing anything?
I can see why you need f5_name and f5_description. Not sure about the rest though.

@ericzji let's review

alexjfisher · 2017-11-22T10:57:13Z

lib/puppet/type/f5_sslcertificate.rb

+require File.expand_path(File.join(File.dirname(__FILE__),'..','..','puppet/property/f5_state.rb'))
+
+Puppet::Type.newtype(:f5_sslcertificate) do
+  @doc = ' Import SSL Certificates'


stray 'space' before Import

alexjfisher · 2017-11-22T10:59:20Z

spec/acceptance/f5_persistencedestaddr/rest_spec.rb

+       match_across_virtuals           => 'enabled',
+       hash_algorithm                  => 'carp',
+       mask                            => '255.255.0.0',
+       timeout                         => '180',


It'd be good to test the type works with actual integers too.

Would you be more specific? Which one to use integers?

Just in general it'd be nice to confirm in the tests that integer properties do actually work with integers. Same with booleans. It'd be nice to check that you can use either 'enabled' or true.

@ericzji what @alexjfisher is suggesting is that the acceptance tests should confirm valid inputs should be tested. In this case, where values can be a word or an integer. Both should be tested in fairness.

alexjfisher · 2017-11-22T11:00:28Z

spec/acceptance/f5_profileclientssl/rest_spec.rb

+    EOS
+    make_site_pp(pp)
+    run_device(:allow_changes => true)
+


Need check for idempotency.

Update for both f5_profileclientssl and f5_profileserverssl

alexjfisher · 2017-11-22T11:01:14Z

spec/acceptance/f5_profileclientssl/rest_spec.rb

+    run_device(:allow_changes => true)
+
+    # pp2=<<-EOS
+    # f5_persistencedestaddr { '/Common/dest_addr1':


Best not to commit large amounts of unrelated commented out code.

@ericzji agreed. Let's get it into a good state or remove it.

alexjfisher · 2017-11-22T11:02:23Z

For the sake of the project's commit history, it would be nice to at least squash all the README commits.

alexjfisher · 2017-11-23T09:00:37Z

lib/puppet/provider/f5_sslcertificate/rest.rb

+
+Puppet::Type.type(:f5_sslcertificate).provide(:rest, parent: Puppet::Provider::F5) do
+
+  def create_message(basename, hash)


create_message is a method from the parent provider that you're overriding. But why?? It's not used anywhere.

alexjfisher · 2017-11-23T11:13:44Z

lib/puppet/provider/f5_sslcertificate/rest.rb

+  def message(object)
+    # Allows us to pass in resources and get all the attributes out
+    # in the form of a hash.
+    message = object.to_hash


This seems to be used in a lot of providers. I've no idea why though. object is already a hash, isn't it?

@ericzji offline please confirm value of object passed in is already hash. Nice find @alexjfisher.

the class for object is Puppet::Type::F5_sslcertificate. Not very clear the structure, but different from hash? @alexjfisher @abrader

abrader

Approval granted on the condition that only small, iterative changes and therefore PRs are submitted in the future.

ericzji added 30 commits October 3, 2017 15:41

Update CHANGELOG.md

da29b83

Acceptance f5_persistencecookie/

c82b135

Acceptance test for f5_persistencedestaddr

9d6e809

Add resource provider for f5_persistencedestaddr

5e0ea8c

Add resource provider f5_persistencecookie

e311211

Acceptance test for f5_persistencehash

e9e686c

Merge branch 'development' of github.com:f5devcentral/f5-puppet into …

c43bcfb

…development

Added resource provider f5_persistencehash

e50da41

Acceptance test

e2a4ada

Merge branch 'development' of github.com:f5devcentral/f5-puppet into …

075b6d4

…development

Acceptance test for f5_persistencesourceaddr

5e53b53

Added resource provider for f5_persistencesourceaddr

5997819

Acceptnance test f5_persistencessl

4bd539b

Added resource for f5_persistencess

4c5690c

Merge branch 'development' of github.com:f5devcentral/f5-puppet into …

e09bdc3

…development

Acceptance test f5_persistenceuniversal

00f7516

Add resource type and provider for f5_persistenceuniversal

aae1fea

Merge branch 'development' of github.com:f5devcentral/f5-puppet into …

a329f30

…development

Add resource for f5_profileclientssl

af8ef9c

Add resource type and provider for f5_datagrou

79683ef

Acceptance test for f5_datagroup

72b238e

Add resource type and provider for f5_profileserverssl

101fad4

Add acceptance test for f5_profileserverssl

105b430

Add resource type and provider for f5_profileclientssl

3d57bea

Add acceptance test for f5_profileserverssl

940284a

Merge branch 'development' of github.com:f5devcentral/f5-puppet into …

115b30d

…development

Minor change

560f91d

Add resource type and provider for f5_profilehttp

33b8d15

Merge branch 'development' of github.com:f5devcentral/f5-puppet into …

182b12d

…development

Add acceptance test for f5_profilehttp

aa4b6e6

alexjfisher reviewed Nov 22, 2017

View reviewed changes

Deleted comment line

7ee2e65

alexjfisher reviewed Nov 23, 2017

View reviewed changes

ericzji added 2 commits December 1, 2017 16:37

Addressed review feedback from @alexjfisher for PR #18

b02d3da

Removed ensurable for providers without destroy method

a0b5db6

abrader approved these changes Dec 15, 2017

View reviewed changes

abrader merged commit 02cdc9c into master Dec 15, 2017


		Puppet::Type.type(:f5_sslcertificate).provide(:rest, parent: Puppet::Provider::F5) do

		def create_message(basename, hash)

Development for new feature types and providers #18

Development for new feature types and providers #18

Conversation

ericzji commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjfisher Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjfisher Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjfisher Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjfisher commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abrader left a comment

Choose a reason for hiding this comment

alexjfisher Nov 22, 2017 •

edited

Loading

alexjfisher Nov 22, 2017 •

edited

Loading

alexjfisher Nov 22, 2017 •

edited

Loading