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

Adding api endpoints for resources to heat api #840

Merged
merged 1 commit into from
Oct 14, 2016
Merged

Adding api endpoints for resources to heat api #840

merged 1 commit into from
Oct 14, 2016

Conversation

drmaas
Copy link
Contributor

@drmaas drmaas commented Oct 3, 2016

No description provided.

@drmaas
Copy link
Contributor Author

drmaas commented Oct 3, 2016

@vinodborole PTAL when you get a chance.

@vinodborole
Copy link
Contributor

Thanks @drmaas for this, the code looks good. Is it possible for you add few test cases for the resource health use case?

@auhlig
Copy link
Member

auhlig commented Oct 4, 2016

In addition to Vinods comment: Could you resolve the conflicts and stick to the import style in core/src/main/java/org/openstack4j/api/Builders.java? @drmaas

@drmaas
Copy link
Contributor Author

drmaas commented Oct 5, 2016

Will address comments.

@drmaas
Copy link
Contributor Author

drmaas commented Oct 6, 2016

@vinodborole @auhlig added tests - PTAL

@vinodborole
Copy link
Contributor

LGTM, @auhlig can you have a look once; especially the json files used for test cases

@auhlig
Copy link
Member

auhlig commented Oct 7, 2016

Yeah. I get what you mean Vinod: d69e213#diff-8159b497a2f70e96643a4d320903a5bbR2 makes no sense.

@auhlig
Copy link
Member

auhlig commented Oct 7, 2016

But the rest of the code looks good.

@drmaas
Copy link
Contributor Author

drmaas commented Oct 7, 2016

@auhlig what makes no sense?

If you are referring to patchWithResponse and postWithResponse, I return an ActionResponse so that the response code can be obtained to check the status, because the responses return no data.

If you are referring to the json files themselves, what would be a better set of test data to use? The resource response is an example of actual metadata returned from a stack resource. The signal json is made up, since I have no context for what a real payload would look like.

Or are you saying that the signal api does not accept a request body?

@auhlig
Copy link
Member

auhlig commented Oct 8, 2016

Hi @drmaas ,
Right. I meant { foo: bar } in the request body I quoted above. Maybe some of the attributes described here http://developer.openstack.org/api-ref/orchestration/v1/index.html#send-a-signal-to-a-resource (see Request Parameters) would make more sense.

While reading the the API documentation I was wondering, where the tenant_id in the path /v1/{tenant_id}/stacks/{stack_name}/{stack_id}/resources/{resource_name}/signal is coming from as the path you're using starts with stacks/...?
I can see that the path lacking the tenant_id is used in the whole ResourcesServiceImpl, so could we check which path is correct?

@drmaas
Copy link
Contributor Author

drmaas commented Oct 10, 2016

@auhlig I tested out all of the endpoints in ResourcesServiceImpl against our Mitaka install. They all do use the tenant (project id) in the URI path that is baked into the client when the client (v2 or v3) is built. This seems to be the pattern for all Openstack4j api calls - automatically use the project id that was specified at client build time. I've seen instances where the docs are wrong.

You're right about no payload needing to be sent - I will update the PR.

providing some tests, return ActionResponse instead of void

remove unused imports

Removing payload for signal api endpoint
@auhlig
Copy link
Member

auhlig commented Oct 11, 2016

Interesting. Thanks for sharing the info. Could you also submit a PR to update the docs? That would be much appreciated!

Besides this PR looks good. What do you think @vinodborole?

@drmaas
Copy link
Contributor Author

drmaas commented Oct 11, 2016

@auhlig http://developer.openstack.org/api-ref/orchestration/v1/index.html#stack-resources looks correct to me (tenant id is in the path for all endpoints) - are you referring to something else?

@auhlig
Copy link
Member

auhlig commented Oct 11, 2016

I'm referring to our docs :)

@drmaas
Copy link
Contributor Author

drmaas commented Oct 11, 2016

@auhlig @vinodborole issue submitted

@auhlig
Copy link
Member

auhlig commented Oct 12, 2016

Thanks. I guess it's ready to merge, right @vinodborole? If so please merge

@auhlig auhlig added this to the 3.0.3 Release milestone Oct 12, 2016
@vinodborole
Copy link
Contributor

Yes @auhlig it is good to merge now, thanks a lot @drmaas for this contribution 👍

@vinodborole vinodborole merged commit a987637 into ContainX:master Oct 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants