Refactor #15

ghoneycutt · 2020-09-27T21:57:11Z

No description provided.

ghoneycutt · 2020-09-27T22:07:59Z

.rubocop.yml

@@ -1,2 +1,4 @@
+Style/TrailingCommaInLiteral:


Opened voxpupuli/voxpupuli-test#31 as I think that's where this file is now managed.

This is merged, though not sure about the release process for that gem. Anyhow, this should probably stay in the commit as it will just get overwritten in the next sync and keep doing the same thing.

igalic

🙋🏻‍♀️

manifests/agent.pp

igalic · 2020-09-28T06:25:45Z

manifests/server.pp

+    if $manage_yarn {
+      yumrepo { 'yarn':
+        ensure   => 'present',
+        baseurl  => 'https://dl.yarnpkg.com/rpm/',


how is this supposed to work for non-rpm systems?

The module only supports EL7

ekohl · 2020-09-28T09:51:50Z

spec/classes/hyperglass_agent_spec.rb

+          is_expected.to contain_file('/opt/hyperglass').with(
+            {
+              'ensure' => 'directory',
+              'owner'  => 'root',
+              'group'  => 'root',
+              'mode'   => '0755',
+            }
+          )


I generally like chaining more. It's compacter and also works nicely if you add other lines like .that_requires()

Suggested change

is_expected.to contain_file('/opt/hyperglass').with(

{

'ensure' => 'directory',

'owner' => 'root',

'group' => 'root',

'mode' => '0755',

}

)

is_expected.to contain_file('/opt/hyperglass').

with_ensure('directory').

with_owner('root').

with_group('root').

with_mode('0755')

I disagree, mostly because I can copy/paste from the actual code and write the tests faster. I did leave the .with_this and .with_that for lines that were not too long.

bastelfreak · 2020-09-28T11:40:36Z

I'm not a big fan of throwing most of the resources into a single class. From the official style guide:

Split your module into public and private classes and defined types where possible. Public classes or defined types should contain the parts of the module meant to be configured or customized by the user, while private classes should contain things you do not expect the user to change via parameters. Separating into public and private classes or defined types helps build reusable and readable code.

alexjfisher · 2020-09-28T12:40:02Z

spec/classes/hyperglass_agent_spec.rb

+              'owner'  => 'hyperglass-agent',
+              'group'  => 'hyperglass-agent',
+              'mode'   => '0700',
+              'notify' => 'Systemd::Unit_file[hyperglass-agent.service]',


Better to use the relationship matchers. https://github.com/rodjek/rspec-puppet#relationship-matchers

The tests will continue to work if the way the relationships are defined are ever refactored.

That's a great idea!

Used those where the .with_ and .that_ were chained. Kept this as is because it makes it harder to read to use multiple formats for the same it block.

spec/classes/hyperglass_server_spec.rb

ghoneycutt · 2020-09-28T17:31:26Z

I'm not a big fan of throwing most of the resources into a single class. From the official style guide:

Split your module into public and private classes and defined types where possible. Public classes or defined types should contain the parts of the module meant to be configured or customized by the user, while private classes should contain things you do not expect the user to change via parameters. Separating into public and private classes or defined types helps build reusable and readable code.

A bunch of private classes just make the code hard to use and understand. I'm emphatically against it on so many levels. Hopefully given all the extra functionality and completeness of the spec tests, the value here is greater than these ideas about splitting code into a bunch of different files.

manifests/server.pp

* Add complete spec test coverage * Default values for hyperglass::server no longer contain bogus devices * Simplify the classes * Make managing users and groups optional * Make each of the service dependencies for the server optional

spec/acceptance/server_spec.rb

ghoneycutt · 2020-10-04T23:17:55Z

Anything else to get this merged?

bastelfreak

I still don't think that it's a good idea to not follow the official style guide and recommendations from Puppet. If we do not agree with them anymore they should be changed instead of being ignored. If others think this is a good approach they shall merge it.

ghoneycutt · 2020-10-07T18:14:06Z

That section of the README is not very prescriptive at all. It does not say you need to scatter your resources across multiple private and public classes. I'm not opposed to doing that and do in the partner module for Sensu (https://github.com/sensu/sensu-puppet). This module is too simple to need all that and if it did, it would not be on ::config, ::install, and ::service lines.

From the docs on classes

https://puppet.com/docs/puppet/6.18/lang_classes.html

Classes generally configure large or medium-sized chunks of functionality, such as all of the packages, configuration files, and services needed to run an application.

vox-pupuli-tasks · 2022-06-18T13:21:08Z

Dear @ghoneycutt, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

ghoneycutt requested review from igalic and bastelfreak September 27, 2020 21:57

ghoneycutt mentioned this pull request Sep 27, 2020

Add Vagrant support #9

Closed

ghoneycutt force-pushed the refactor branch from f5d2ce8 to 97a6fca Compare September 27, 2020 22:03

ghoneycutt commented Sep 27, 2020

View reviewed changes

igalic reviewed Sep 28, 2020

View reviewed changes

ekohl reviewed Sep 28, 2020

View reviewed changes

alexjfisher reviewed Sep 28, 2020

View reviewed changes

spec/classes/hyperglass_server_spec.rb Outdated Show resolved Hide resolved

alexjfisher reviewed Sep 28, 2020

View reviewed changes

spec/classes/hyperglass_server_spec.rb Outdated Show resolved Hide resolved

bastelfreak reviewed Sep 28, 2020

View reviewed changes

manifests/server.pp Outdated Show resolved Hide resolved

ghoneycutt force-pushed the refactor branch from 97a6fca to 1520ba1 Compare September 28, 2020 17:53

ghoneycutt added 2 commits September 28, 2020 14:47

Refactor

82e2a7a

* Add complete spec test coverage * Default values for hyperglass::server no longer contain bogus devices * Simplify the classes * Make managing users and groups optional * Make each of the service dependencies for the server optional

Add Vagrant support

699e83e

ghoneycutt force-pushed the refactor branch from 1520ba1 to 699e83e Compare September 28, 2020 18:48

ghoneycutt mentioned this pull request Sep 28, 2020

manage_depended_services parameter #3

Open

ekohl reviewed Sep 30, 2020

View reviewed changes

spec/acceptance/server_spec.rb Show resolved Hide resolved

ekohl mentioned this pull request Sep 30, 2020

Add support to store environment variables as facts voxpupuli/voxpupuli-acceptance#13

Merged

bastelfreak reviewed Oct 5, 2020

View reviewed changes

vox-pupuli-tasks bot added the merge-conflicts label Jun 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor #15

Refactor #15

ghoneycutt commented Sep 27, 2020

ghoneycutt Sep 27, 2020

ghoneycutt Sep 28, 2020

igalic left a comment

igalic Sep 28, 2020

ghoneycutt Sep 28, 2020

ekohl Sep 28, 2020

ghoneycutt Sep 28, 2020

bastelfreak commented Sep 28, 2020

alexjfisher Sep 28, 2020 •

edited

Loading

ghoneycutt Sep 28, 2020

ghoneycutt Sep 29, 2020

ghoneycutt commented Sep 28, 2020

ghoneycutt commented Oct 4, 2020

bastelfreak left a comment

ghoneycutt commented Oct 7, 2020

vox-pupuli-tasks bot commented Jun 18, 2022

Refactor #15

Are you sure you want to change the base?

Refactor #15

Conversation

ghoneycutt commented Sep 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igalic left a 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

bastelfreak commented Sep 28, 2020

alexjfisher Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghoneycutt commented Sep 28, 2020

ghoneycutt commented Oct 4, 2020

bastelfreak left a comment

Choose a reason for hiding this comment

ghoneycutt commented Oct 7, 2020

vox-pupuli-tasks bot commented Jun 18, 2022

alexjfisher Sep 28, 2020 •

edited

Loading