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

Switch to using qpid bind #177

Merged
merged 1 commit into from
Mar 27, 2017
Merged

Switch to using qpid bind #177

merged 1 commit into from
Mar 27, 2017

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Mar 24, 2017

No description provided.

@ehelms
Copy link
Member Author

ehelms commented Mar 24, 2017

Relies on theforeman/puppet-qpid#52

ssl_cert => $client_cert,
queue => $candlepin_event_queue,
exchange => 'event',
Copy link
Member Author

Choose a reason for hiding this comment

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

Not ideal to hardcode, but did that for now since this should get more re-factoring as we split things out.

Copy link
Member

Choose a reason for hiding this comment

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

You could introduce a $candlepin_event_exchange and hardcode the default there. That at least makes it easier to override later on.

@ehelms
Copy link
Member Author

ehelms commented Mar 24, 2017

Updated to add a variable for the candlepin exchange as a placeholder

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Maybe also refactor the qpid-config execs to use qpid::config_cmd?

@@ -25,8 +26,9 @@
require => Service['qpidd'],
logoutput => true,
} ~>
qpid::bind_event { ['entitlement.created', 'entitlement.deleted', 'pool.created', 'pool.deleted', 'compliance.created']:
qpid::bind { ['entitlement.created', 'entitlement.deleted', 'pool.created', 'pool.deleted', 'compliance.created']:
Copy link
Member

Choose a reason for hiding this comment

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

This should now also be qpid::config::bind

@ehelms ehelms force-pushed the use-bind branch 2 times, most recently from 030005a to 8f8def5 Compare March 24, 2017 17:44
@ehelms
Copy link
Member Author

ehelms commented Mar 24, 2017

Refactored!

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Much easier to read!

ssl_cert => $client_cert,
} ~>
Copy link
Member

Choose a reason for hiding this comment

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

This can be a simple -> I think.

@ehelms
Copy link
Member Author

ehelms commented Mar 24, 2017

Updated per comments

@ekohl
Copy link
Member

ekohl commented Mar 27, 2017

Since theforeman/puppet-qpid#52 is now merged, could you rebase to see if the tests pass?

@ehelms
Copy link
Member Author

ehelms commented Mar 27, 2017

@ekohl that did the trick, thanks.

@ehelms ehelms merged commit 8faa722 into theforeman:master Mar 27, 2017
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.

None yet

2 participants