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

Support for ipmievd. #1

Closed
razorsedge opened this issue Sep 4, 2013 · 6 comments
Closed

Support for ipmievd. #1

razorsedge opened this issue Sep 4, 2013 · 6 comments

Comments

@razorsedge
Copy link
Contributor

I have an outstanding PR with another module on GitHub (logicminds/bmclib#3). I would like to see this implemented in your module. Since I am not yet a fan of the new class pattern that is all the rage, how would you suggest I go about implementing a PR? (Or is this something that you could implement quickly?)

@jhoblitt
Copy link
Owner

@razorsedge I'm sorry for not responding to this issue sooner. I didn't even know https://github.com/logicminds/bmclib existed. I had been planning to eventually implement similar functionality for controlling BMC user accounts. I wonder if the modules can't be merged...

The PR you submitted was fine and I'd like to make a 1.1.0 release to the forge soon.

How do you feel about changing the class definition for ipmi::service to something like this?

class ipmi::service (
  $ensure_ipmi    = 'running',
  $ensure_ipmievd = 'stopped,
) {
  validate_re($ensure_ipmi, '^running$|^stopped$')
  validate_re($ensure_ipmievd, '^running$|^stopped$')

@razorsedge
Copy link
Contributor Author

@jhoblitt No worries. GitHub has been a little flaky with notifications over the last few months.

I would consider not merging the bmclib as I think the creator's intent is to build a lot of different providers for those types. I still need to contact him to confirm. The main problem I have right now is that the types just don/t work and I made a first (unsuccessful) run at fixing them over the weekend. My RubyFoo is very weak. :-(

That class define looks fine. Did you want me to send a PR?

@razorsedge
Copy link
Contributor Author

Actually, the proposed change to the class will still have to deal with the service's enable parameter matching the running or stopped state. That tends to be why I use the case statement. I've never really tried to think up a more compact way to do it.

@jhoblitt
Copy link
Owner

How about I hack together a PR of the API I'm proposing and we can iterate from there? Is there any reason ipmievd shouldn't be started by default? Is there ever a case where you'd want to run ipmievd without the ipmi service having been started first to load drivers?

@razorsedge
Copy link
Contributor Author

ipmievd ships with ipmitool and is disabled by default. I would keep it that way as other vendor tools might be unhappy if it were polling the local BMC. I would expect the ipmi service to be running before ipmievd unless the user was pointing ipmievd at a networked BMC.

@jhoblitt
Copy link
Owner

This was resolved by PR #5.

jhoblitt pushed a commit that referenced this issue Jun 28, 2016
Test stubs and option to change interface type
mojothemonkey added a commit to mojothemonkey/puppet-ipmi that referenced this issue Aug 3, 2016
I find some of my servers have ipmi running on channel jhoblitt#2 only (plugged into different physical port maybe - who knows).
so the impi_ipaddress etc facts (which come from channel jhoblitt#1) are not set. as channel jhoblitt#1 is not configured.

instead of always taking channel jhoblitt#1 for these, could we take 'the first found LAN channel' instead? (asside: this is the behaviour when running 'ipmitool lan print' without picking a channel).

have put in a suggested change for this. think it would work to only set each of the ipmi_<name> facts if not set so far.

cheers,
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

No branches or pull requests

2 participants