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

Refactor rabbitmq_user provider (mpolenchuk) #598

Merged
merged 2 commits into from
Sep 28, 2017

Conversation

wyardley
Copy link
Contributor

@wyardley wyardley commented Sep 4, 2017

This is an attempt to rework #460 by @mpolenchuk (keeping original attribution).
Could use careful review, rebasing was pretty hairy, and not sure I got everything.

As of now, failing at least one test.

@wyardley wyardley requested a review from ekohl September 4, 2017 05:56
@wyardley wyardley force-pushed the rework_pull_460 branch 2 times, most recently from a3e3f98 to 96bd913 Compare September 4, 2017 16:58
@wyardley wyardley added the enhancement New feature or request label Sep 8, 2017
@wyardley wyardley changed the title Refactor rabbitmq_user provider [WIP] Refactor rabbitmq_user provider Sep 8, 2017
@wyardley wyardley force-pushed the rework_pull_460 branch 3 times, most recently from 80e8fda to bcd0294 Compare September 12, 2017 00:12
@wyardley wyardley added the bug Something isn't working label Sep 12, 2017
@wyardley wyardley force-pushed the rework_pull_460 branch 5 times, most recently from 4a2a470 to 5f91749 Compare September 12, 2017 05:35
@wyardley
Copy link
Contributor Author

@ekohl @hunner I think this is ready for review, and would very much appreciate a review on this one if at all possible. In particular, I think the tests could use some improvement, especially for the change password situation.

However, at a functional level, I think this is working(ish), and an improvement.

@wyardley wyardley changed the title [WIP] Refactor rabbitmq_user provider Refactor rabbitmq_user provider Sep 12, 2017
@wyardley wyardley changed the title Refactor rabbitmq_user provider Refactor rabbitmq_user provider (mpolenchuk) Sep 12, 2017
end

it do
provider.check_password('secret')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is doing anything useful, but need to come up with a better test here.

@wyardley
Copy link
Contributor Author

@mpolenchuk - don't know if you've got time to review, but let me know if this looks sane. I'm still trying to come up with a better test case for the password change bit.

wyardley pushed a commit to wyardley/puppet-rabbitmq that referenced this pull request Sep 12, 2017

def should_to_s(_newvalue = @should)
'<new password>'
end
end

newproperty(:admin) do
Copy link
Member

Choose a reason for hiding this comment

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

You can use newproperty(:admin, boolean: true, parent: Parent::Property::Boolean) and remove the body (except desc). Then you can use true booleans in the provider. You might need a require 'puppet/property/boolean but I'm not sure about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a good idea, but I feel like it should be implemented separately, and then we can review all the type parameters at once, and adjust any that we can switch to true booleans / integers / etc in one 'breaking' changeset.

Copy link
Member

Choose a reason for hiding this comment

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

That's fair

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like the existing code is (in some cases) as simple as:

  newparam(:force) do
    defaultto(:false)
    newvalues(:true, :false)
  end

which actually seems simpler than the suggested code in https://docs.puppet.com/puppet/5.1/custom_types.html, and without that extra require. Also, the puppet boolean type allows 'true' yes, and various other things, which seems kind of regressive.

@wyardley wyardley force-pushed the rework_pull_460 branch 4 times, most recently from e231bcf to aa01efd Compare September 13, 2017 08:18
@wyardley wyardley removed the bug Something isn't working label Sep 13, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e6407e2 on wyardley:rework_pull_460 into ** on voxpupuli:master**.

2 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e6407e2 on wyardley:rework_pull_460 into ** on voxpupuli:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e6407e2 on wyardley:rework_pull_460 into ** on voxpupuli:master**.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.959% when pulling 343006b on wyardley:rework_pull_460 into 3a5268d on voxpupuli:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.959% when pulling 343006b on wyardley:rework_pull_460 into 3a5268d on voxpupuli:master.

@coveralls
Copy link

coveralls commented Sep 13, 2017

Coverage Status

Coverage increased (+0.3%) to 78.959% when pulling 343006b on wyardley:rework_pull_460 into 3a5268d on voxpupuli:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.959% when pulling fe41806 on wyardley:rework_pull_460 into 3a5268d on voxpupuli:master.

1 similar comment
@coveralls
Copy link

coveralls commented Sep 13, 2017

Coverage Status

Coverage increased (+0.3%) to 78.959% when pulling fe41806 on wyardley:rework_pull_460 into 3a5268d on voxpupuli:master.

@coveralls
Copy link

coveralls commented Sep 13, 2017

Coverage Status

Coverage increased (+0.3%) to 78.959% when pulling 50358fc on wyardley:rework_pull_460 into 3a5268d on voxpupuli:master.

@wyardley
Copy link
Contributor Author

@ekohl: the shared example seems to get executed after the other elements in the block, which is causing an ordering issue.
For example

    it_behaves_like 'an idempotent resource'

    it 'does not have the user' do
      shell('rabbitmqctl list_users -q') do |r|
        expect(r.stdout).not_to match(%r{dan\s+})
      end
    end

results in

  destroy user resource
    does not have the user (FAILED - 1)
    behaves like an idempotent resource
localhost $ scp /var/folders/0z/yrnglm6n1l5_q19dv1yy0w3c0000gn/T/beaker20170913-21543-xcrm0o centos-7-x64:/tmp/apply_manifest.pp.Kz0FfQ {:ignore => }
      applies with no errors
localhost $ scp /var/folders/0z/yrnglm6n1l5_q19dv1yy0w3c0000gn/T/beaker20170913-21543-ez5qfe centos-7-x64:/tmp/apply_manifest.pp.BUFI7K {:ignore => }
      applies a second time without changes

It's failing because the test for the user being gone runs before the shared example runs.

@coveralls
Copy link

coveralls commented Sep 13, 2017

Coverage Status

Coverage increased (+0.3%) to 78.959% when pulling 6974325 on wyardley:rework_pull_460 into 3a5268d on voxpupuli:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.959% when pulling e3730f6 on wyardley:rework_pull_460 into 3a5268d on voxpupuli:master.

1 similar comment
@coveralls
Copy link

coveralls commented Sep 14, 2017

Coverage Status

Coverage increased (+0.3%) to 78.959% when pulling e3730f6 on wyardley:rework_pull_460 into 3a5268d on voxpupuli:master.

@wyardley wyardley added this to the 7.1.0 milestone Sep 28, 2017
@hunner hunner merged commit e3730f6 into voxpupuli:master Sep 28, 2017
hunner added a commit that referenced this pull request Sep 28, 2017
Refactor rabbitmq_user provider (mpolenchuk)
@wyardley wyardley deleted the rework_pull_460 branch September 29, 2017 04:48
@wyardley wyardley modified the milestones: 7.1.0, v7.1.0, 6.0.0 Sep 30, 2017
Slm0n87 pushed a commit to Slm0n87/puppet-rabbitmq that referenced this pull request Mar 7, 2019
Refactor rabbitmq_user provider (mpolenchuk)
cegeka-jenkins pushed a commit to cegeka/puppet-rabbitmq that referenced this pull request Mar 26, 2021
Refactor rabbitmq_user provider (mpolenchuk)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants