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

fix so repo download occurs before install, need to anchor #9

Merged
merged 1 commit into from
Feb 5, 2015

Conversation

serenapotts3
Copy link

see my issue raised for more details

@serenapotts3
Copy link
Author

@msimonin Have you had a chance to look at this PR, we have issues deploying cassandra without it

@msimonin
Copy link
Owner

msimonin commented Jan 9, 2015

Hi @serenakeating,

I'll do it soon.

@pkallos
Copy link

pkallos commented Jan 26, 2015

👍 i am experiencing this issue when testing with the tests/Vagrantfile

@msimonin
Copy link
Owner

Anchoring seems a reasonnable way to do it. contain function seems to recent to be used here.
Thanks @serenakeating !

@@ -19,6 +19,8 @@
key => $key_id,
key_source => $gpgkey,
pin => $pin,
before => Anchor['cassandra::repo::end'],
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we write :

before => Package['dsc']

to force the source list to be updated before the package installation ?

@yeungda
Copy link

yeungda commented Feb 5, 2015

Hi, @msimonin, I've been working with @serenakeating on this change. I hope this comment helps answer your question and allows us to get closer to fixing the ordering issue.

TLDR; Both options are fine! I prefer containment.

I'll just try and discuss the two solutions a bit. Hopefully you'll understand the differences and be able to choose.

First, the proposed containment solution.

In Class['cassandra'], the code asserts:

Class['cassandra::repo'] -> Class['cassandra::install']

By containing Class['cassandra::repo::debian'] within Class['cassandra::repo'], we allow the above assertion to work as expected.

This means that in the future, you can add additional assertions about the order of Class['cassandra::repo'] without worrying about how it works inside. It will be easy to add them to Class['cassandra'], so your assertions will be at the top level of your class hierarchy. You can also rename the cassandra::repo::* classes, with minimal impact on other classes in your codebase.

Now, for the before => Package['dsc'] solution:

In Class['cassandra'], the code asserts:

Class['cassandra::repo'] -> Class['cassandra::install']

This assertion isn't actually honoured at all, since all of the resources Class['cassandra::repo'] includes are uncontained classes.

Without containment, maintainers will probably need to remember not to depend on Class['cassandra::repo'].

We then ask Class['cassandra::repo'] to assert:

Class['cassandra::repo::debian'] -> Package['dsc']

This introduces a coupling between Class['cassandra::repo'] and Class['cassandra:install']. So it increases the risk that by changing Class['cassandra:install'] you will break Class['cassandra:repo'].
This also means that any future ordering requirements between classes in your codebase will need to be specified in a similar way, increasing coupling further.
It also leaves you with two different ways of asserting that repo should happen before install, one of which that doesn't work, one of which does.

If you go for this, I would suggest that you remove the assertion:

Class['cassandra::repo'] -> Class['cassandra::install']

Having said all that, which option would you prefer to merge? We'll change the Pull Request to suit.

EDIT: Regarding class design issues like the coupling, I think R.I.Pienaar explains these issues much better than I could here:

https://www.devco.net/archives/2012/12/13/simple-puppet-module-structure-redux.php

@msimonin
Copy link
Owner

msimonin commented Feb 5, 2015

@yeungda you convinced me :)

msimonin added a commit that referenced this pull request Feb 5, 2015
fix so repo download occurs before install, need to anchor
@msimonin msimonin merged commit 0bb1d6a into msimonin:master Feb 5, 2015
@msimonin
Copy link
Owner

msimonin commented Feb 6, 2015

Btw I'm interesting in using pythos. Is there a manifest that I can use ?

@serenapotts3
Copy link
Author

As far as I know there is not a public one, we ended up writing our own. Though not sure if we can opensource it as it was for the company I work for. There are some manifests in the excscale github project but they are more for cassandra.

It was very simple though, install the package, run the service and configure the /etc/pithos/pithos.yaml via an pithos.yaml.erb file which I just copied and parameterised.

The only extra thing I had to do was initalise the schemas:
exec {'pithos initialisation':
command => "pithos -a install-schema",
path => '/bin:/usr/bin',
require => File['/etc/pithos/pithos.yaml'],
refreshonly => true,
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants