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

Allow value to be object type in ServiceConfig #1061

Merged
merged 3 commits into from
Aug 7, 2017

Conversation

ekasitk
Copy link
Contributor

@ekasitk ekasitk commented Jul 12, 2017

This PR allows values in ServiceConfig to be an integer or any other types than just a string. So the generated JSON is correctly formatted according to the type of value.

@vinodborole
Copy link
Contributor

@ekasitk can you please share the use case for this change request?

@ekasitk
Copy link
Contributor Author

ekasitk commented Jul 13, 2017

When setting service parameters, values can be string or number.

  ServiceConfig hdfsConf = Builders.serviceConfig()
                              .set("dfs.namenode.logging.level","info")
                              .set("dfs.replication",1);
 Cluster cluster = Builders.cluster().name("test")
                        .addServiceConfig("HDFS",hdfsConf);

This PR permits ServiceConfig.set() to accept number and generate the json like:
{
"name" : "test",
"cluster_configs" : {
"HDFS" : {
"dfs.namenode.logging.level" : "info",
"dfs.replication" : 1
}
},
"hadoop_version" : "1.6.2"
}

Otherwise, all values will always be treated as string, i.e. "dfs.replication" : "1", which is not complied well to the API. Ref: https://developer.openstack.org/api-ref/data-processing/?expanded=show-details-of-a-cluster-detail.

@auhlig
Copy link
Member

auhlig commented Jul 14, 2017

Makes sense. Or not @vinodborole?
But a test for a more complex config would be nice.

@auhlig auhlig added this to the 3.1.0 Release milestone Jul 14, 2017
@vinodborole
Copy link
Contributor

I agree @auhlig as per the openstack doc this change request makes sense. A test for a more complex config will be necessary in order to be sure nothing else breaks. @ekasitk is it possible for you to add this test case along with this PR?

@ekasitk
Copy link
Contributor Author

ekasitk commented Jul 19, 2017

@vinodborole @auhlig
I can add some test cases but it will take a while.

@vinodborole
Copy link
Contributor

@ekasitk sure not a problem

@ekasitk
Copy link
Contributor Author

ekasitk commented Aug 5, 2017

@vinodborole @auhlig
I've added some test cases for this PR. Please kindly check.

@vinodborole
Copy link
Contributor

LGTM @auhlig what do you think?

@auhlig auhlig merged commit 95218cb into ContainX:master Aug 7, 2017
@auhlig
Copy link
Member

auhlig commented Aug 7, 2017

LGTM. Many thanks @ekasitk

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