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.

Sign up for GitHub

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

Jump to bottom

Add address field to memcached plugin #853

Merged
merged 1 commit into from
Oct 20, 2018
Merged

Add address field to memcached plugin #853

merged 1 commit into from
Oct 20, 2018

Conversation

mrunge
Copy link
Contributor

@mrunge mrunge commented Sep 11, 2018

As addition to the host field, there is also the address setting,
which is helpful sometimes.

Pull Request (PR) description

This also adds the address field to the memcached plugin.

bastelfreak
bastelfreak reviewed Sep 11, 2018
View reviewed changes
templates/plugin/memcached.conf.erb
@@ -1,9 +1,16 @@
<Plugin memcached>
Copy link
Member

Choose a reason for hiding this comment

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

hey @mrunge, are you interested in turning this into an epp template?

Copy link
Member

Choose a reason for hiding this comment

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

@mrunge We prefer epp for new templates going forward, but will merge this for now.

bastelfreak
bastelfreak reviewed Sep 11, 2018
View reviewed changes
manifests/plugin/memcached.pp Outdated
@@ -1,7 +1,10 @@
# https://collectd.org/wiki/index.php/Plugin:memcached
class collectd::plugin::memcached (
$ensure = 'present',
Hash $instances = {'default' => {'host' => '127.0.0.1', 'port' => 11211 } },
Hash $instances = {'default' => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please enforce the structure of the hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by 'enforcing the structure' here? I'm not sure that I can follow you.

@bastelfreak bastelfreak added enhancement New feature or request needs-work not ready to merge just yet labels Sep 11, 2018
@mrunge
Copy link
Contributor Author

mrunge commented Sep 11, 2018

Ok, now the tests report a valid issue, I'll submit an update asap.

@mrunge
Copy link
Contributor Author

mrunge commented Sep 12, 2018

I see the tests failing, but not connected to the change.

On my setup, tests passed, and on 5 of the CI runners;

@mrunge
Copy link
Contributor Author

mrunge commented Sep 12, 2018

Ok, the error has been

# ArgumentError:
#   wrong number of arguments (given 0, expected 2)
#   ./vendor/bundle/ruby/2.5.0/gems/beaker-puppet-1.3.0/lib/beaker-puppet/helpers/host_helpers.rb:17:in 'stat'

@mrunge
Copy link
Contributor Author

mrunge commented Sep 13, 2018

I'm still not sure what's happening in the tests. It seems not connected.

@mrunge mrunge mentioned this pull request Sep 13, 2018
Closed
@mrunge
Copy link
Contributor Author

mrunge commented Sep 13, 2018

@bastelfreak see #855 for the same CI issue. The other patch does nothing. Any idea how to fix CI here?

@bastelfreak
Copy link
Member

I restarted the tests here, it should now work.

@mrunge
Copy link
Contributor Author

mrunge commented Sep 17, 2018

@bastelfreak Any news from tests here? The failure is different, but still failing.

@bastelfreak
Copy link
Member

This is another bug in beaker-puppet/beaker :(

@mrunge
Copy link
Contributor Author

mrunge commented Sep 27, 2018

Any ideas or how to continue?

@bastelfreak
Copy link
Member

@alexjfisher do you know if there was any progress on the beaker issue? I lost track.

@alexjfisher
Copy link
Member

I think puppetlabs/beaker-puppet#59 might have caused a regression?

@alexjfisher
Copy link
Member

But I can't see how or why yet. I'll do some more testing.

@alexjfisher
Copy link
Member

beaker failures fixed in #858

@bastelfreak
Copy link
Member

@mrunge can you rebase please?

@mrunge
Add address field to memcached plugin
4ce93c0 
As addition to the host field, there is also the address setting,
which is helpful sometimes.
@mrunge
Copy link
Contributor Author

mrunge commented Oct 15, 2018

@bastelfreak done.
It seems it passes now :-)

@juniorsysadmin juniorsysadmin added needs-work not ready to merge just yet and removed needs-rebase needs-work not ready to merge just yet labels Oct 15, 2018
@mrunge
Copy link
Contributor Author

mrunge commented Oct 19, 2018

Is anything missing here from my side?

@juniorsysadmin juniorsysadmin removed the needs-work not ready to merge just yet label Oct 20, 2018
@juniorsysadmin juniorsysadmin merged commit 9624672 into voxpupuli:master Oct 20, 2018
@mrunge
Copy link
Contributor Author

mrunge commented Oct 22, 2018

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bastelfreak bastelfreak bastelfreak left review comments

@juniorsysadmin juniorsysadmin juniorsysadmin approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mrunge @bastelfreak @alexjfisher @juniorsysadmin