-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 PolicyReadByName for API #6615
Conversation
819db3d
to
6a667ed
Compare
It'd come in handy as we're currently working around the issue by filtering on client side: https://github.com/scalp42/consul-cookbook/blob/scalp42/libraries/consul_policy.rb#L61-L75 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be ready now to implement the changes PolicyRead
RPC function here
You will want to check if the ID is empty and if so call state.ACLPolicyReadByName
instead of state.ACLPolicyReadByID
That should be the last bit to get this functional.
Then all thats needed is a couple tests:
agent/consul/acl_endpoint_test.go - exercise the RPC func to ensure reading by name works. Mostly copy the normal policy read test and change it to lookup the policy by name.
agent/acl_endpoint_test.go - add another subtest similar to this one
b2a8b68
to
9c46c9e
Compare
Hey @mkeeler, Thank you so much for the help on getting this ready. Greatly appreciate all the help you provided on this. Let me know if there's anything else you think is required here. Else, I'll go ahead rebase and squash this. |
@abaez I apologize for letting this linger for so long. This looks good to me now but will require resolving some conflicts with the current master. It looks like its just the three files github is showing.
I know its been a long time and would be willing to fix these conflicts on your branch if you no longer have the time or inclination to do so yourself and then get this merged. |
Hey @mkeeler, Sure I can go ahead and update this during the weekend. And no worries. I know your team is artfully busy. These things happen, even with small changes. Though wouldn't mind some push on go-discover side. |
9c46c9e
to
40054e2
Compare
@i0rek , @mkeeler, can either of you review over the change? I believe it is in good enough shape now for squash and merge. Let me know if there's anything you need on my end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abaez Thanks for all your work on this. I am going to approve this but since we have already started the release machinery for v1.7.2 this wont make it in until 1.7.3.
While writing an update to the python library consulate for Consul's ACL gmr/consulate#123, I noticed that Consul's API was missing the PolicyReadByName API function. Seeing as how it's fairly simple fix, here's a small patch I made to the API to add the ability. Covers at the read by name issue on #5962 brought up.
Let me know if there's any error or modification I should do here.