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

Wrong return type of disable/enable service actions #1076

Merged
merged 7 commits into from
Aug 17, 2017

Conversation

baseman79
Copy link
Contributor

@vinodborole
Copy link
Contributor

@baseman79 I don't see a reason why the return type should not get parsed with Service class. It does satisfies all the parameters; can you share any exceptions or errors you get?

https://github.com/ContainX/openstack4j/blob/master/core/src/main/java/org/openstack4j/model/compute/ext/Service.java

@vinodborole
Copy link
Contributor

@baseman79 I don't there would be need for this PR anymore; until there is something we are trying to fix here from the older code.

@baseman79
Copy link
Contributor Author

@vinodborole
I don't see a reason why the return type should not get parsed with Service class. It does satisfies all the parameters; can you share any exceptions or errors you get?

sorry, are you saying the old code with return type Sevice.Status will work fine?

@vinodborole
Copy link
Contributor

@baseman79 Yes it should

@baseman79
Copy link
Contributor Author

@vinodborole,

here is the return from OS:

2 < 200
2 < Connection: keep-alive
2 < Content-Length: 81
2 < Content-Type: application/json
2 < Date: Tue, 08 Aug 2017 14:00:10 GMT
2 < X-Compute-Request-Id: req-eacf5035-08c9-43dc-9b5c-74d5c88dbae0
{"service": {"status": "enabled", "binary": "nova-compute", "host": "somehost"}}

and the value I got back by using the old code

s	Service$Status  (id=81)	
	name	"UNRECOGNIZED" (id=87)	
	ordinal	2

@baseman79
Copy link
Contributor Author

Hi @vinodborole,

Just to be clear, I made a very simple call to disable a host with the current master code (return type is Service.Status)

Service.Status s = oc.compute().services().disableService("nova-compute", hostName);

OS reponsed

2 < 200
2 < Connection: keep-alive
2 < Content-Length: 81
2 < Content-Type: application/json
2 < Date: Tue, 08 Aug 2017 14:00:10 GMT
2 < X-Compute-Request-Id: req-eacf5035-08c9-43dc-9b5c-74d5c88dbae0
{"service": {"status": "enabled", "binary": "nova-compute", "host": "somehost"}}

And I verified the host is disabled, but the return value I got for s is UNRECOGNIZED

Do you still think this pr is not needed?

Thanks,

@vinodborole
Copy link
Contributor

@baseman79 thank you for debuging this

@auhlig what do you think?

@auhlig
Copy link
Member

auhlig commented Aug 11, 2017

We're talking https://developer.openstack.org/api-ref/compute/#disable-scheduling-for-a-compute-service and https://developer.openstack.org/api-ref/compute/#enable-scheduling-for-a-compute-service, right?
Both return a service, so this PR makes perfect sense.
It also looks like this is currently not tested, so I'd like to ask if you could add a unit test.

@auhlig auhlig added the bug label Aug 11, 2017
@auhlig auhlig added this to the 3.1.0 Release milestone Aug 11, 2017
@baseman79
Copy link
Contributor Author

@auhlig that's correct. I will add an unit test, thx

@baseman79
Copy link
Contributor Author

@auhlig @vinodborole anyone?

@auhlig
Copy link
Member

auhlig commented Aug 16, 2017

Sorry for the delay. I'll take a look in the evening.

@baseman79
Copy link
Contributor Author

no worries, thanks @auhlig

@auhlig auhlig merged commit b13feaa into ContainX:master Aug 17, 2017
@auhlig
Copy link
Member

auhlig commented Aug 17, 2017

Thanks for contributing @baseman79

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants