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 ACL support #257

Merged
merged 4 commits into from
Dec 22, 2015
Merged

Add ACL support #257

merged 4 commits into from
Dec 22, 2015

Conversation

bdclark
Copy link
Contributor

@bdclark bdclark commented Dec 13, 2015

This is a rough draft of a resource to provide ACL support using the diplomat gem as an API client. It's not ready to merge yet since there's no unit tests, etc... however I wanted to throw it out there for comments and suggestions.

Since most of the work is being handled by diplomat there's really not much to it. It's passing the included integration tests, is idempotent, notifies correctly, etc.

A few noteworthy points:

  • Since it requires a chef_gem there needs to be a way to install it. Rather than doing anything fancy in the resource I decided to leave it up to the user to ensure the gem is installed first. A consul::client_gem recipe is included with the cookbook to do the needful.
  • The diplomat version is not pinned, but an attribute exists to set the version if desired.
  • I'm requiring diplomat in each of the action blocks rather than the top of the resource to prevent the gem from needing to be installed at compile-time (is this best-practice?).
  • There is no :update... just :create and :delete. After tinkering with the Consul API for awhile it looks like the only difference between create and update in Consul's API is the fact that create can be called without an ID (it'll create the ACL with a random UUID if ID is not provided). So, to keep things simple/predictable the create endpoint is used for create/update and an ID is required.

Comments? Thanks!

@codecov-io
Copy link

Current coverage is 51.36%

Merging #257 into master will decrease coverage by -21.28% as of 5b2e69b

@@            master    #257   diff @@
======================================
  Files            6       6       
  Stmts          329     329       
  Branches         0       0       
  Methods          0       0       
======================================
- Hit            239     169    -70
  Partial          0       0       
- Missed          90     160    +70

Review entire Coverage Diff as of 5b2e69b

Powered by Codecov. Updated on successful CI builds.

@johnbellone
Copy link
Contributor

This looks good to me.

johnbellone added a commit that referenced this pull request Dec 22, 2015
@johnbellone johnbellone merged commit 6e6c96b into sous-chefs:master Dec 22, 2015
@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