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

Implement APIv1 behaviors #201

Merged
merged 2 commits into from
Feb 25, 2016
Merged

Implement APIv1 behaviors #201

merged 2 commits into from
Feb 25, 2016

Conversation

danielsdeleo
Copy link
Contributor

No description provided.

@danielsdeleo
Copy link
Contributor Author

Have this PR up to fix test issue with ChefFS mode: chef/chef-server#753


def get(request)
result = super
user_data = FFI_Yajl::Parser.parse(result[2], :create_additions => false)
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW RestBase defines parse_json and to_json helpers. (I discovered this too late for my last PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch.

@jrunning
Copy link
Contributor

👍

@danielsdeleo
Copy link
Contributor Author

@chef/client-dev-tools Ready for review.

if request.api_v0?
principal_data
else
{ "principals" => [ principal_data ] }
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is both a user and a client with the given name, should we return them both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's correct, according to my reading of the code here: https://github.com/chef/chef-server/blob/218579549024b26b3b97c9958503ff3651e3fb16/src/oc_erchef/apps/oc_chef_wm/src/chef_wm_named_principal.erl#L92-L102

OTOH, there's no pedant test for that, and it's an existing bug, and I probably won't have time to fix all that. So my inclination is to leave it as is until the problem comes up again.

@jkeiser
Copy link
Contributor

jkeiser commented Feb 25, 2016

@danielsdeleo one question, but looks good!

@jkeiser
Copy link
Contributor

jkeiser commented Feb 25, 2016

👍

@danielsdeleo danielsdeleo merged commit 2b255fd into master Feb 25, 2016
@danielsdeleo danielsdeleo deleted the api-v1 branch February 26, 2016 00:02
@thommay thommay added Type: Enhancement Adds new functionality. and removed enhancement labels Jan 24, 2017
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Enhancement Adds new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants