You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Noorul Islam K M <no...@github.com> on 2013/09/17 19:51:52 UTC
[jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
You can merge this Pull Request by running:
git pull https://github.com/noorul/jclouds-chef jclouds-285
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-chef/pull/24
-- Commit Summary --
* JCLOUDS-285: Add name field in CookbookDefinition domain
-- File Changes --
M core/src/main/java/org/jclouds/chef/domain/CookbookDefinition.java (26)
M core/src/main/java/org/jclouds/chef/functions/ParseCookbookDefinitionFromJsonv10.java (9)
M core/src/main/java/org/jclouds/chef/functions/ParseCookbookDefinitionListFromJsonv10.java (15)
M core/src/test/java/org/jclouds/chef/ChefApiExpectTest.java (29)
M core/src/test/java/org/jclouds/chef/functions/ParseCookbookDefinitionFromJsonv10Test.java (2)
M core/src/test/java/org/jclouds/chef/functions/ParseCookbookDefinitionListFromJsonv10Test.java (6)
A core/src/test/resources/env_cookbooks.json (20)
-- Patch Links --
https://github.com/jclouds/jclouds-chef/pull/24.patch
https://github.com/jclouds/jclouds-chef/pull/24.diff
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #66](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/66/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24#issuecomment-24736863
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Andrew Phillips <no...@github.com>.
> @@ -43,6 +43,13 @@
>
> @Override
> public CookbookDefinition apply(HttpResponse response) {
> - return parser.apply(response).values().iterator().next();
> + Map<String, CookbookDefinition> result = parser.apply(response);
Why can't the parser set the name of the CookbookDefinition itself?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6427970
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Andrew Phillips <no...@github.com>.
> @@ -156,6 +158,33 @@ public void testListNodesInEnvironmentReturns404() {
> assertTrue(nodes.isEmpty(), String.format("Expected nodes to be empty but was: %s", nodes));
> }
>
> + public void testListCookbooksInEnvironmentReturns2xx() {
> + ChefApi api = requestSendsResponse(
> + signed(HttpRequest.builder() //
> + .method("GET") //
> + .endpoint("http://localhost:4000/environments/dev/cookbooks") //
> + .addHeader("X-Chef-Version", ChefApiMetadata.DEFAULT_API_VERSION) //
> + .addHeader("Accept", MediaType.APPLICATION_JSON).build()), //
> + HttpResponse.builder().statusCode(200)
> + .payload(payloadFromResourceWithContentType("/env_cookbooks.json", MediaType.APPLICATION_JSON)) //
> + .build());
> + Set<CookbookDefinition> cookbooks = api.listCookbooksInEnvironment("dev");
> + assertEquals(cookbooks.size(), 2);
> + }
> +
> + public void testListCookbooksInEnvironmentReturns404() {
Could we rename this to indicate what it's actually testing? E.g. "testListCookbooks404ReturnsEmptyCollection" or "...ReturnsNull" or whatever..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6428008
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -56,20 +62,32 @@ public Builder versions(Iterable<Version> versions) {
> return this;
> }
>
> + public Builder from(CookbookDefinition def) {
> + this.url = def.getUrl();
> + this.versions.addAll(def.getVersions());
Also set the `name` field?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6430257
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Noorul Islam K M <no...@github.com>.
> @@ -82,6 +100,7 @@ public URI getUrl() {
> public int hashCode() {
> final int prime = 31;
> int result = 1;
> + result = prime * result + ((name == null) ? 0 : url.hashCode());
My bad
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6430503
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -483,6 +484,19 @@ public void testListNodesInEnvironment() {
> assertTrue(!nodeList.isEmpty());
> }
>
> + @Test(dependsOnMethods = "testCreateNewCookbook")
> + public void testListCookbooksInEnvironment() throws Exception {
> + Set<CookbookDefinition> cookbooks = api.listCookbooksInEnvironment("_default");
> + assertNotNull(cookbooks);
> + CookbookDefinition cookbook = cookbooks.iterator().next();
> + String cookbookName = cookbook.getName();
> + assertNotNull(cookbookName);
> + assertEquals(cookbookName, PREFIX);
This live test could be executed in any environment. Are we sure that the first cookbook returned will be the one created by this class? Can we assert instead that the returned set "contains" that cookbook?
Also, add a proper message to the assertions.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6458927
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -47,6 +48,18 @@
>
> @Override
> public Set<CookbookDefinition> apply(HttpResponse response) {
> - return Sets.newLinkedHashSet(parser.apply(response).values());
> + Set<Map.Entry<String, CookbookDefinition>> result = parser.apply(response).entrySet();
> + return Sets.newHashSet(Iterators.transform(result.iterator(), new Function<Map.Entry<String, CookbookDefinition>, CookbookDefinition>() {
`result` is already an `Iterable`, so there is no need to call `iterator()` if you use `Iterables.transform`.
Why the change from `LinkedHashSet` to `HashSet` ?
Prefer using static imports for the `Sets` and `Iterables` stuff.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6425831
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #58](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/58/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24#issuecomment-24650330
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Noorul Islam K M <no...@github.com>.
> @@ -43,6 +43,13 @@
>
> @Override
> public CookbookDefinition apply(HttpResponse response) {
> - return parser.apply(response).values().iterator().next();
> + Map<String, CookbookDefinition> result = parser.apply(response);
>From IRC
<pre>
<nacx> if we want gson to do all the deserialization, we need to change the
model and add a new entity that holds the name and contains the current
cookbook definition
<nacx> to match the json structure
<nacx> if we just want to add the name to the current CookbookDefinition,
which would look cleaner
</pre>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6428360
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Noorul Islam K M <no...@github.com>.
> @@ -43,6 +43,13 @@
>
> @Override
> public CookbookDefinition apply(HttpResponse response) {
> - return parser.apply(response).values().iterator().next();
> + Map<String, CookbookDefinition> result = parser.apply(response);
Agreed and Done!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6429171
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #56](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/56/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24#issuecomment-24648901
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -43,6 +43,13 @@
>
> @Override
> public CookbookDefinition apply(HttpResponse response) {
> - return parser.apply(response).values().iterator().next();
> + Map<String, CookbookDefinition> result = parser.apply(response);
The problem is that the JSON structure does not 100% match the Java domain object model, and we would need a new entity that had the name of the cookbook and a CookbookDefinition inside.
It feels more natural to have the name in the CookbookDefinition itself, and that's the reason why it is manually added after parsing the Map. (See the JSON file for the Expect test and the CookbookDefinition Java class to see the differences in the structure)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6428355
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Ignasi Barrera <no...@github.com>.
All live tests are passing. Merged!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24#issuecomment-24737645
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Noorul Islam K M <no...@github.com>.
> @@ -483,6 +484,19 @@ public void testListNodesInEnvironment() {
> assertTrue(!nodeList.isEmpty());
> }
>
> + @Test(dependsOnMethods = "testCreateNewCookbook")
> + public void testListCookbooksInEnvironment() throws Exception {
> + Set<CookbookDefinition> cookbooks = api.listCookbooksInEnvironment("_default");
> + assertNotNull(cookbooks);
> + CookbookDefinition cookbook = cookbooks.iterator().next();
> + String cookbookName = cookbook.getName();
> + assertNotNull(cookbookName);
> + assertEquals(cookbookName, PREFIX);
That is a possibility.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6459008
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #62](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/62/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24#issuecomment-24730136
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Noorul Islam K M <no...@github.com>.
> @@ -56,20 +62,32 @@ public Builder versions(Iterable<Version> versions) {
> return this;
> }
>
> + public Builder from(CookbookDefinition def) {
> + this.url = def.getUrl();
> + this.versions.addAll(def.getVersions());
Or do you want me to set the name as "name" as it is not already available?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6430512
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #60](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/60/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24#issuecomment-24659246
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -56,20 +62,32 @@ public Builder versions(Iterable<Version> versions) {
> return this;
> }
>
> + public Builder from(CookbookDefinition def) {
> + this.url = def.getUrl();
> + this.versions.addAll(def.getVersions());
Just set the name as you set the other properties. If the parameter comes with the name populated, just set it.
You don't know if the name is available or not (you don't know where the parameter comes from, people could use this builder method in their app to convert the CookbookDefinition to a mutable object to be able to modify it, for example).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6430706
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #67](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/67/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24#issuecomment-24737189
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds-chef #406](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-chef/406/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24#issuecomment-24653138
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Noorul Islam K M <no...@github.com>.
> @@ -43,6 +43,13 @@
>
> @Override
> public CookbookDefinition apply(HttpResponse response) {
> - return parser.apply(response).values().iterator().next();
> + Map<String, CookbookDefinition> result = parser.apply(response);
For that we have to change the CookbookDefinition domain representation.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6428052
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Andrew Phillips <no...@github.com>.
> @@ -47,6 +49,18 @@
>
> @Override
> public Set<CookbookDefinition> apply(HttpResponse response) {
> - return Sets.newLinkedHashSet(parser.apply(response).values());
Again, why can't the parser already set the name of the CookbookDefinition correctly?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6427981
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -43,6 +43,13 @@
>
> @Override
> public CookbookDefinition apply(HttpResponse response) {
> - return parser.apply(response).values().iterator().next();
> + Map<String, CookbookDefinition> result = parser.apply(response);
Another option would be to tell Gson how to deserialize a CookbookDefinition by adding a new "CookbookDefinitionAdapter" in the ChefParser module, but IMO that would be more complex as we have to deal with different parsers for Chef versions 0.9 and later versions.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6428392
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #55](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/55/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24#issuecomment-24608957
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Andrew Phillips <no...@github.com>.
> @@ -43,6 +43,13 @@
>
> @Override
> public CookbookDefinition apply(HttpResponse response) {
> - return parser.apply(response).values().iterator().next();
> + Map<String, CookbookDefinition> result = parser.apply(response);
Ah, OK. In that case, perhaps add a `from(CookbookDefinition)` method to the Builder? Then we could shorten this and the other invocation to something like:
```
return CookbookDefinition.Builder.from(def).name(cookbookName).build();
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6428717
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Andrew Phillips <no...@github.com>.
> @@ -156,6 +158,33 @@ public void testListNodesInEnvironmentReturns404() {
> assertTrue(nodes.isEmpty(), String.format("Expected nodes to be empty but was: %s", nodes));
> }
>
> + public void testListCookbooksInEnvironmentReturns2xx() {
Again, could we rename this to explain the use case?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6428014
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #59](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/59/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24#issuecomment-24653251
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Noorul Islam K M <no...@github.com>.
> @@ -56,20 +62,32 @@ public Builder versions(Iterable<Version> versions) {
> return this;
> }
>
> + public Builder from(CookbookDefinition def) {
> + this.url = def.getUrl();
> + this.versions.addAll(def.getVersions());
Do you want me to change the signature to from(CookbookDefinition def, String cookbookName) ?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6430497
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Ignasi Barrera <no...@github.com>.
One last thing. I've noticed we didn't have a live test for the `listCookbooksInEnvironment` methods. Could you add the tests in the [BaseChefApiLiveTest](https://github.com/jclouds/jclouds-chef/blob/master/core/src/test/java/org/jclouds/chef/internal/BaseChefApiLiveTest.java) and assert that the responses has indeed the name populated?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24#issuecomment-24720293
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Noorul Islam K M <no...@github.com>.
> @@ -47,6 +48,18 @@
>
> @Override
> public Set<CookbookDefinition> apply(HttpResponse response) {
> - return Sets.newLinkedHashSet(parser.apply(response).values());
> + Set<Map.Entry<String, CookbookDefinition>> result = parser.apply(response).entrySet();
> + return Sets.newHashSet(Iterators.transform(result.iterator(), new Function<Map.Entry<String, CookbookDefinition>, CookbookDefinition>() {
Done!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6427592
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Noorul Islam K M <no...@github.com>.
> @@ -156,6 +158,33 @@ public void testListNodesInEnvironmentReturns404() {
> assertTrue(nodes.isEmpty(), String.format("Expected nodes to be empty but was: %s", nodes));
> }
>
> + public void testListCookbooksInEnvironmentReturns2xx() {
> + ChefApi api = requestSendsResponse(
> + signed(HttpRequest.builder() //
> + .method("GET") //
> + .endpoint("http://localhost:4000/environments/dev/cookbooks") //
> + .addHeader("X-Chef-Version", ChefApiMetadata.DEFAULT_API_VERSION) //
> + .addHeader("Accept", MediaType.APPLICATION_JSON).build()), //
> + HttpResponse.builder().statusCode(200)
> + .payload(payloadFromResourceWithContentType("/env_cookbooks.json", MediaType.APPLICATION_JSON)) //
> + .build());
> + Set<CookbookDefinition> cookbooks = api.listCookbooksInEnvironment("dev");
> + assertEquals(cookbooks.size(), 2);
> + }
> +
> + public void testListCookbooksInEnvironmentReturns404() {
Let me remove this test case, so that I add this in the next PR which will contain the fix to return empty list on 404.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6428072
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -82,6 +100,7 @@ public URI getUrl() {
> public int hashCode() {
> final int prime = 31;
> int result = 1;
> + result = prime * result + ((name == null) ? 0 : url.hashCode());
Change to `name.hashCode()`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6430264
Re: [jclouds-chef] JCLOUDS-285: Add name field in CookbookDefinition
domain (#24)
Posted by Ignasi Barrera <no...@github.com>.
> + .build());
> + Set<CookbookDefinition> cookbooks = api.listCookbooksInEnvironment("dev");
> + assertEquals(cookbooks.size(), 2);
> + }
> +
> + public void testListCookbooksInEnvironmentReturns404() {
> + ChefApi api = requestSendsResponse(
> + signed(HttpRequest.builder() //
> + .method("GET") //
> + .endpoint("http://localhost:4000/environments/dev/cookbooks") //
> + .addHeader("X-Chef-Version", ChefApiMetadata.DEFAULT_API_VERSION) //
> + .addHeader("Accept", MediaType.APPLICATION_JSON).build()), //
> + HttpResponse.builder().statusCode(404)
> + .build());
> + Set<CookbookDefinition> cookbooks = api.listCookbooksInEnvironment("dev");
> + assertNull(cookbooks);
Just noticed this when reading the Expect test. It is not related to this pull request, but `list` methods should never return `null` ([null is evil](https://code.google.com/p/guava-libraries/wiki/UsingAndAvoidingNullExplained) and we try to avoid using it in jclouds). [This response parser](https://github.com/jclouds/jclouds-chef/blob/master/core/src/main/java/org/jclouds/chef/ChefApi.java#L203) should be changed to `EmptySetOnNotFoundOr404`, to behave as all other list methods. Do you want to take this too?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/24/files#r6425878