Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure multiple instances may be created with the default package. #348

Merged
merged 4 commits into from
Feb 18, 2020

Conversation

surprisingb
Copy link

In a scenario with multiple instances, the services' files won't be created if the default haproxy package is used.
The title itself MUST be different between the instances, but the package could be the same for all of them.

@lionce
Copy link

lionce commented Apr 18, 2019

Hello @surprisingb ,

Thank you for your contribution, but it looks like the unit tests are failing. In order to merge your PR, please fix the errors. Please let us know if you need any help.

@surprisingb
Copy link
Author

Hi @lionce,
I've looked at the code but I'm not really sure about this.
The error thrown on the unit tests are like this:

1) haproxy::instance_service when on any platform with title group1 and custom settings installs the customhaproxy package
     Failure/Error: content => template($haproxy_unit_template),

     Puppet::PreformattedError:
       Evaluation Error: Error while evaluating a Resource Statement, Evaluation Error: Error while evaluating a Function Call, Could not find template '' (file: /home/travis/build/puppetlabs/puppetlabs-haproxy/spec/fixtures/modules/haproxy/manifests/instance_service.pp, line: 107, column: 20) (line: 2) on node travis-job-2538e14a-d52e-4ac6-9729-124276d3de8b.c.eco-emissary-99515.internal

but the $haproxy_unit_template parameter is undef by default, and I didn't change anything related to this.

@lionce
Copy link

lionce commented May 7, 2019

Hey @surprisingb ,

I'll have a look on this!

Cheers!

@carabasdaniel
Copy link

Hi @surprisingb,

Could you please rebase this to the latest changes on master ?

Thank you.

@surprisingb
Copy link
Author

I rebased my repo but the error it's still the same.

@carabasdaniel
Copy link

Hi @surprisingb,

After looking through the failing test, apparently this change forces the haproxy_unit_template parameter (https://github.com/puppetlabs/puppetlabs-haproxy/blob/master/manifests/instance_service.pp#L42) to become required at https://github.com/puppetlabs/puppetlabs-haproxy/blob/master/manifests/instance_service.pp#L112.

So to get this working as it should could you try, as a first step, setting the haproxy_unit_template.

I hope this gives you a clear track on getting the tests passing.

Please feel free to reach out if you have difficulties with this.

Thanks.

@surprisingb
Copy link
Author

Hi @carabasdaniel, I defined a default service unit template, I'm still getting an error:

1) haproxy::instance_service when on any platform with title group1 and custom settings installs the customhaproxy package
     Failure/Error:
       subject.should contain_package('customhaproxy').with(
         'ensure' => 'present',
       )
     
     Puppet::Error:
       Could not find resource 'Service[haproxy-haproxy]' in parameter 'before' (file: /home/travis/build/puppetlabs/puppetlabs-haproxy/spec/fixtures/modules/haproxy/manifests/instance_service.pp, line: 120) on node travis-job-abfb9ec9-3745-4ec5-9880-c406d71f2401.c.eco-emissary-99515.internal

Line 120 refers to before => Service["haproxy-${title}"],, but the title should be group1, why is it looking for Service[haproxy-haproxy]?

I also just found this note that I didn't see before, and maybe this specific use case is what the note is referring to... What do you think?

@lionce
Copy link

lionce commented Jul 15, 2019

Hello @surprisingb , in order to fix the tests (Could not find resource 'Service[haproxy-haproxy]' in parameter 'before'), you should add a pre_condition block with service {'haproxy-#{title}': }

let(:pre_condition) do
      <<-PUPPETCODE
        service {'haproxy-#{title}': }
      PUPPETCODE
    end

The error is pointing to haproxy because title is still haproxy - check this line .
You also should update the paths when title is different than haproxy e.g. this line
I hope this helps you on getting the tests passing!
If you need any help, feel free to contact us. :)

Cheers!

@lionce
Copy link

lionce commented Nov 7, 2019

Hello @surprisingb ,

Please let us know if you're still working on this PR! If you need any help don't hesitate to ask. Thanks!

Cheers!

Giacomo Bonfiglioli and others added 3 commits January 13, 2020 11:06
The class won't create multiple instances if the default package is used
Add default unit template to instance service manifest.
@lionce
Copy link

lionce commented Jan 13, 2020

Hello @surprisingb ,

i've updated your branch to include latest updates from master and fixed the failing tests.

Cheers!

@michaeltlombardi michaeltlombardi changed the title Modify check on standard instance Ensure multiple instances may be created with the default package. Feb 12, 2020
@codecov-io
Copy link

codecov-io commented Feb 18, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@0ec0a02). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #348   +/-   ##
=========================================
  Coverage          ?   16.66%           
=========================================
  Files             ?        1           
  Lines             ?        6           
  Branches          ?        0           
=========================================
  Hits              ?        1           
  Misses            ?        5           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ec0a02...7fa7946. Read the comment docs.

@sheenaajay sheenaajay merged commit 321a7bc into puppetlabs:master Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants