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

Add provider for consul acls #220

Closed
wants to merge 1 commit into from

Conversation

kamaradclimber
Copy link
Contributor

This patch introduces acl provider for consul.
Most consul resources can injected as configuration file, acls however
have to interacted with through http api.
ConsulClient library exposes minimum helpers for such interaction.

This patch is based on the work of @InformatiQ

@codecov-io
Copy link

Current coverage is 64.12%

Merging #220 into master will increase coverage by +8.85% as of ea5dc9d

@@            master    #220   diff @@
======================================
  Files            6       7     +1
  Stmts          322     354    +32
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            178     227    +49
  Partial          0       0       
+ Missed         144     127    -17

Review entire Coverage Diff as of ea5dc9d

Powered by Codecov. Updated on successful CI builds.

@kamaradclimber
Copy link
Contributor Author

@johnbellone is there anything I can improve to get this merged ?

@kamaradclimber
Copy link
Contributor Author

@johnbellone could you give me some feedback on this change ?

This patch introduces acl provider for consul.
Most consul resources can injected as configuration file, acls however
have to interacted with through http api.
ConsulClient library exposes minimum helpers for such interaction.

This patch is based on the work of @InformatiQ
@johnbellone
Copy link
Contributor

@kamaradclimber Apologies for not looking at this sooner.

I am wondering if we should be using an existing Ruby library for the Consul HTTP client? There seem to be a few on Rubygems.org already.

@client ||= Chef::Consul::Client.new(nil, node['consul']['ports']['http'], node['consul']['acl_master_token'])
end

def load_current_resource

This comment was marked as outdated.

@johnbellone
Copy link
Contributor

I am also going to need more tests here. I took a look at the code and am a little wary of executing arbitrary ruby blocks. Could you please explain what you're trying to do there? Maybe we could work on refactoring it.

@kamaradclimber
Copy link
Contributor Author

thanks for you feedback. We still use the pre1.0 version of this cookbook in my organisation so I relied on rspec to validate the patch (version 1.0 does not work on windows 2012R2 probably due to a chef bug)

I'll have a look at your comments in the next days.

@kamaradclimber
Copy link
Contributor Author

I can answer about using them gem: @InformatiQ has done the PR against the diplomat gem (WeAreFarmGeek/diplomat#46) that contains the code.

since the diplomat PR needs to be improved, let's put this one on hold until we are able to directly use diplomat gem.

@lock
Copy link

lock bot commented Apr 25, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants