Fix rubocop offenses #8

timdeluxe · 2023-01-02T14:12:23Z

Fixing rubocop offenses, so that the modulesync branch CI workflows run green

ekohl · 2023-01-02T14:14:49Z

lib/puppet/provider/host/augeas.rb

@@ -57,14 +59,12 @@ def self.instances

  def self.prefetch(resources)
    targets = []
-    resources.each do |name, resource|
+    resources.each do |_name, resource|


Isn't this better?

Suggested change

resources.each do |_name, resource|

resources.values do |resource|

ekohl · 2023-01-02T14:15:59Z

lib/puppet/provider/host/augeas.rb

      targets << target(resource) unless targets.include? target(resource)
    end
-    hosts = targets.inject([]) { |hosts,target| hosts += get_resources({:target => target}) }
+    hosts = targets.reduce([]) { |_acc, elem| hosts += get_resources({ target: elem }) }


It's weird to use reduce but then don't use the accumulator and this is likely to introduce a bug. This is probably because it's shadowing the outer variable. I think this is what's intended.

Suggested change

hosts = targets.reduce([]) { |_acc, elem| hosts += get_resources({ target: elem }) }

hosts = targets.reduce([]) { |acc, elem| acc += get_resources({ target: elem }) }

It was done by rubocop auto_correct, will check your suggestion

ekohl · 2023-01-02T14:16:45Z

lib/puppet/provider/host/augeas.rb

@@ -88,21 +88,17 @@ def create
      end

      # comment property only available in Puppet 2.7+


Perhaps we can assume Puppet 2.7+ by now and drop the conditional.

ekohl · 2023-01-02T14:18:20Z

lib/puppet/provider/mailalias/augeas.rb

    }
-    return nil unless malias[:name] = aug.get("#{apath}/name")
+    return nil unless malias[:name] == aug.get("#{apath}/name")


This looks like a change, but probably a bugfix. I'd separate this to its own commit.

It was also a rubocop offense, why separate it?

Because it's a very different thing. The old version is essentially:

malias[:name] = aug.get("#{apath}/name") return nil unless malias[:name]

This means malias[:name] was always overwritten.

The new version never overwrites malias[:name]. If any code depends on that behavior, you're breaking it.

I think a Rubocop compatible change is:

Suggested change

return nil unless malias[:name] == aug.get("#{apath}/name")

return nil unless (malias[:name] = aug.get("#{apath}/name"))

ekohl · 2023-01-02T14:19:20Z

lib/puppet/provider/mailalias/augeas.rb

+    resources.each do |_name, resource|
      targets << target(resource) unless targets.include? target(resource)
    end
    maliases = []
    targets.each do |target|
-      maliases += get_resources({:target => target})
+      maliases += get_resources({ target: target })
    end
-    maliases = targets.inject([]) { |malias ,target| maliases += get_resources({:target => target}) }
+    maliases = targets.reduce([]) { |_acc, elem| maliases += get_resources({ target: elem }) }


Same comments as for the other provider.

timdeluxe · 2023-01-02T14:46:24Z

@ekohl I am trying to fix stuff here as you requested in voxpupuli/puppet-augeasproviders_core#41 (comment) - but i missed to mark this as a draft (sorry), which i did now. The fails in @bastelfreak PR (#5) highlighted rubocop problems, so i focussed on that. Will try to fix other stuff now (also what you mentioned) and mark this as ready for review, when i think it makes sense.

timdeluxe · 2023-01-03T17:51:46Z

@ekohl I need your help please if you have any chance to do so, i am really stuck. Have a look at the failing tests (https://github.com/voxpupuli/puppet-augeasproviders_base/actions/runs/3823507437/jobs/6504759037 for example).
It complaints about a not defined provider method for the host/mailalias type. Looks like the host and mailalias types are not loaded at all (the "nil:NilClass" in the error lets me assume that). I am aware, that those have moved from the core to separate modules. However those modules are defined in the .fixtures.yaml and get installed into the spec/fixtures/modules directory. I even picked the spec_helper_local from the augeasproviders_core module, which includes the setting of the modulepath, also that does not change anything.
Maybe also the rubocop fixes introduced an error now causing the augeas provider this module tries to add to the type to not load, but then at least the types should be known (and not get nil:NilClass)...
Can you give me a hint? I want to help, but i am lost now :-(

ekohl · 2023-01-03T18:02:14Z

I think that in 81aa2c3#diff-89eebfcbc0f14b6d989517837ca1e94fce4e2ce9a03233641cd936f2b8d2ed94 the spec_helper changes that were removed actually broke things.

If I add this to spec/spec_helper.rb I get further:

$LOAD_PATH.unshift(
  File.join(__dir__, 'fixtures', 'modules', 'augeasproviders_core', 'lib'),
  File.join(__dir__, 'fixtures', 'modules', 'host_core', 'lib'),
  File.join(__dir__, 'fixtures', 'modules', 'mailalias_core', 'lib'),
)

It then fails on the reliance on mocha where you should use rspec-mocks.

From the top of my head, you'll need to do things like:

# Mocha
FileTest.stubs(:exist?).returns false
FileTest.stubs(:exist?).with('/etc/hosts').returns true
# rspec-mocks
allow(FileTest).to receive(:exist?).and_return(false)
allow(FileTest).to receive(:exist?).with('/etc/hosts').and_return(true)

ekohl · 2023-01-05T14:26:16Z

.github/workflows/ci.yml

-  puppet:
-    name: Puppet
-    uses: voxpupuli/gha-puppet/.github/workflows/basic.yml@v1
+  setup_matrix:


Why have you copied the whole job definition? If you need libaugeas-dev to be installed then you can use additional_packages in .sync.yml. I just updated https://github.com/voxpupuli/puppet-augeasproviders_core to do the same.

I also updated #5. I don't have time to work on it further, but how I would structure it:

Disable Rubocop in that PR (see https://github.com/voxpupuli/gha-puppet#basic-tests)

Get that PR passing

Continue this PR to enable Rubocop

That was the background (looking at how it was implemented for augeasproviders_core).

However i give up for this now. All in all this is too complex for me. I am not that aware how to handle modulesync.
With loading "ruby-augeas" gem in Gemfile i can get at least get the spec tests to do "something", but i do not really understand the errors it produces (also i do not know how to tell modulesync to have that module still in place after a new modulesync). Something is wrong with self.prefetch, even in the old version (without the rubocop fixes), but also without the prefetch method at all i get errors i do not really understand.

I like to contribute, but this module is a really hard one and stuff for the absolute specialists :-/

I leave this draft open, if anyone wants to pick up parts of what i did so far.

So I did a little more, but now I got a bit stuck.

@timdeluxe are you interested in joining Vox Pupuli so you can work on the shared modulesync branch instead?

However i give up for this now. All in all this is too complex for me. I am not that aware how to handle modulesync.

What I typically do is:

Clone https://github.com/voxpupuli/modulesync_config

Run it in noop on the repo so the git checkout is there

Iterate in offline mode

So more concrete, what I have is (some of it is preference and doesn't matter here, like the git clone command):

# Get a working modulesync config setup git clone https://github.com/voxpupuli/modulesync_config ~/dev/voxpupuli-modulesync_config -o upstream cd ~/dev/voxpupuli-modulesync_config bundle install # Check out the specific module # -f is a filter, so augeasproviders will match multiple modules bundle exec msync update --noop -f augeasproviders_base cd modules/voxpupuli/augeasproviders_base # Iterate here as you normally would - it's just a regular git checkout

Now if I need to make a modulesync change, I want it to just run the templates so I use offline mode for that. Offline basically means that it doesn't use git for anything.

cd ~/dev/voxpupuli-modulesync_config bundle exec msync update --offline -f augeasproviders_base

Often I have multiple terminals open so I can easily switch back and forth. I hope that helps.

@ekohl

@timdeluxe are you interested in joining Vox Pupuli so you can work on the shared modulesync branch instead?

Thanks for the offer, but as you can see, i am struggling with general stuff. I am by far not a ruby professional yet. Puppet modules are one of many topics i deal with at work and in my free time. We (my work team and me) are open to contribute to open source projects, but i would like to continue that for voxpupuli like i did in the past. This way i can't destroy something on my own and one of you specialists always look over before something gets merged or released broken.

Hey @timdeluxe 👋 !

Do not consider joining Vox Pupuli as something that requires you to deliver something, but rather as a recognition of your work for the project, with the advantage of having the ability to work collaboratively with other Vox Pupuli members on a project branch without the hassle of forking the repo and allowing external contributors one by one.

Vox Pupuli is a community. This mean that you are not going to be alone and if you are not comfortable with something (afraid to break things?), just don't do it 😄 … or ask other members which will be happy to help / confirm that it is fine / give advice. Everybody in Vox Pupuli does the best they can with the time they have. Sometimes we break things, and when it happen we do our best to fix it. Peer review help avoiding such situation, but as long as it is not abuse, its okay.

If you don't feel it at the moment, of course there is no issue, and you are welcome to continue helping Vox Pupuli as an "external contributor"! I just want you to know that you do not need to be a Ruby expert to join Vox Pupuli 😉.

bastelfreak and others added 6 commits December 2, 2021 13:48

modulesync 5.1.0

81aa2c3

delete .sync.yml

da239ce

Update from voxpupuli modulesync_config

5849de9

modulesync 5.3.0

5ddc571

modulesync 5.4.0

1c3436a

Fix rubocop offenses

171061d

ekohl reviewed Jan 2, 2023

View reviewed changes

timdeluxe marked this pull request as draft January 2, 2023 14:29

Update references to previous module author

a23050d

timdeluxe added 3 commits January 2, 2023 15:49

Code improvements after rubocop auto_correct

40dad4e

Code improvements after rubocop auto_correct

097b69a

Fix unit-tests

2aad8cf

timdeluxe added 5 commits January 4, 2023 16:42

Update module dependencies

bde28c2

Fix spec tests

2c87b06

Install libaugeas-dev in CI workflow

74aa0f6

Fix provider code

fd27353

Fix rubocop offenses

a8f3635

ekohl reviewed Jan 5, 2023

View reviewed changes

ekohl force-pushed the modulesync branch 4 times, most recently from 0a25409 to 70a1dd7 Compare January 5, 2023 16:09

bastelfreak deleted the branch voxpupuli:modulesync January 6, 2023 14:18

bastelfreak closed this Jan 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rubocop offenses #8

Fix rubocop offenses #8

timdeluxe commented Jan 2, 2023

ekohl Jan 2, 2023

ekohl Jan 2, 2023

timdeluxe Jan 2, 2023

ekohl Jan 2, 2023

ekohl Jan 2, 2023

timdeluxe Jan 2, 2023

ekohl Jan 2, 2023

ekohl Jan 2, 2023

timdeluxe commented Jan 2, 2023

timdeluxe commented Jan 3, 2023

ekohl commented Jan 3, 2023

ekohl Jan 5, 2023

ekohl Jan 5, 2023

timdeluxe Jan 5, 2023

ekohl Jan 5, 2023

ekohl Jan 5, 2023

timdeluxe Jan 10, 2023

smortex Jan 11, 2023

	resources.each do \|_name, resource\|
	resources.values do \|resource\|

	hosts = targets.reduce([]) { \|_acc, elem\| hosts += get_resources({ target: elem }) }
	hosts = targets.reduce([]) { \|acc, elem\| acc += get_resources({ target: elem }) }

		@@ -88,21 +88,17 @@ def create
		end

		# comment property only available in Puppet 2.7+

	return nil unless malias[:name] == aug.get("#{apath}/name")
	return nil unless (malias[:name] = aug.get("#{apath}/name"))

Fix rubocop offenses #8

Fix rubocop offenses #8

Conversation

timdeluxe commented Jan 2, 2023

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

timdeluxe commented Jan 2, 2023

timdeluxe commented Jan 3, 2023

ekohl commented Jan 3, 2023

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