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/07 08:49:14 UTC
[jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
You can merge this Pull Request by running:
git pull https://github.com/noorul/jclouds-chef jclouds-265
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-chef/pull/13
-- Commit Summary --
* JCLOUDS-265: Add listEnvironmentNodes API in ChefService
-- File Changes --
M core/src/main/java/org/jclouds/chef/ChefService.java (3)
M core/src/main/java/org/jclouds/chef/internal/BaseChefService.java (10)
A core/src/main/java/org/jclouds/chef/strategy/ListEnvironmentNodes.java (45)
A core/src/main/java/org/jclouds/chef/strategy/internal/ListEnvironmentNodesImpl.java (109)
A core/src/test/java/org/jclouds/chef/strategy/internal/GetEnvironmentNodesImplLiveTest.java (80)
-- Patch Links --
https://github.com/jclouds/jclouds-chef/pull/13.patch
https://github.com/jclouds/jclouds-chef/pull/13.diff
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Noorul Islam K M <no...@github.com>.
> @@ -177,4 +177,7 @@
>
> @SinceApiVersion("0.10.0")
> Iterable<? extends Environment> listEnvironmentsNamed(Iterable<String> names);
> +
> + @SinceApiVersion("0.10.0")
> + Iterable<? extends Node> listEnvironmentNodes(String environmentName);
Does that mean, this patch is good to go?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13/files#r6233035
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-chef #361](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-chef/361/) 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/13#issuecomment-24152468
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -177,4 +177,7 @@
>
> @SinceApiVersion("0.10.0")
> Iterable<? extends Environment> listEnvironmentsNamed(Iterable<String> names);
> +
> + @SinceApiVersion("0.10.0")
> + Iterable<? extends Node> listEnvironmentNodes(String environmentName);
I see your point and agree. Taking a look at the ChefApi, I've seen several methods that should be renamed to follow a more java-friendly naming too. If you are agree, I'd leave this pull request unchanged, and I'll open a new one cleaning up the Chef Api: I'll improve the method naming, group methods better in the code, improve the javadoc, and remove the redundant methods in the ChefService that only delegate to direct `Iterables.find` calls and don't add value. This way we'll have only one single PR to cleanup the ChefApi and the ChefService. Agree?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13/files#r6239346
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-chef #346](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-chef/346/) 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/13#issuecomment-24040997
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Noorul Islam K M <no...@github.com>.
@nacx What do you mean by squashing the commits?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-24145985
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Andrew Phillips <no...@github.com>.
> + this.strategy = injector.getInstance(ListEnvironmentNodesImpl.class);
> + creater.execute(prefix, ImmutableSet.<String> of());
> + creater.execute(prefix + 1, ImmutableSet.<String> of());
> + }
> +
> + @AfterClass(groups = { "integration", "live" })
> + @Override
> + protected void tearDown() {
> + api.deleteNode(prefix);
> + api.deleteNode(prefix + 1);
> + super.tearDown();
> + }
> +
> + @Test
> + public void testExecute() {
> + assert size(strategy.execute("_default")) > 0;
Use `assertTrue(size(strategy.execute("_default")) > 0, "Expected one or more elements")`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13/files#r6228587
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #26](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/26/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-23983339
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Andrew Phillips <no...@github.com>.
> @@ -177,4 +177,7 @@
>
> @SinceApiVersion("0.10.0")
> Iterable<? extends Environment> listEnvironmentsNamed(Iterable<String> names);
> +
> + @SinceApiVersion("0.10.0")
> + Iterable<? extends Node> listEnvironmentNodes(String environmentName);
> The api implementation is named listEnvironmentNodes.
You mean in Chef itself? Or in jclouds?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13/files#r6232655
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #31](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/31/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-24041015
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Andrew Phillips <no...@github.com>.
Apart from the question about the method naming, looks good to me ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-24028374
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Andrew Phillips <no...@github.com>.
> @@ -177,4 +177,7 @@
>
> @SinceApiVersion("0.10.0")
> Iterable<? extends Environment> listEnvironmentsNamed(Iterable<String> names);
> +
> + @SinceApiVersion("0.10.0")
> + Iterable<? extends Node> listEnvironmentNodes(String environmentName);
As far as I'm concerned, I'd still like to know whether we think "listNodesInEnvironment" is a clearer name than "listEnvironmentNodes". Wonder if @nacx has some thoughts on that..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13/files#r6233245
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-chef #343](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-chef/343/) FAILURE
Looks like there's a problem with this pull request
[(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/13#issuecomment-24039406
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Andrew Phillips <no...@github.com>.
> @@ -177,4 +177,7 @@
>
> @SinceApiVersion("0.10.0")
> Iterable<? extends Environment> listEnvironmentsNamed(Iterable<String> names);
> +
> + @SinceApiVersion("0.10.0")
> + Iterable<? extends Node> listEnvironmentNodes(String environmentName);
> Agree?
+1. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13/files#r6240219
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-chef #360](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-chef/360/) 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/13#issuecomment-24151817
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Andrew Phillips <no...@github.com>.
@noorul Now we have **10** commits. I think we want to get this down to **1** commit ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-24152255
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Andrew Phillips <no...@github.com>.
> @@ -177,4 +177,7 @@
>
> @SinceApiVersion("0.10.0")
> Iterable<? extends Environment> listEnvironmentsNamed(Iterable<String> names);
> +
> + @SinceApiVersion("0.10.0")
> + Iterable<? extends Node> listEnvironmentNodes(String environmentName);
Well, the thing I'm thinking about here is that "listX" might be expected to return X. I know that "listEnvironmentNodes" ends in "Nodes", but to have "listEnvironments" return environments and a method that looks really similar return something else might be a bit confusion.
In fact, I might even go so far as to suggest that this could be
```
Iterable<Node> listNodes(String environmentName, ...)
```
That, to me, would align most neatly on the Java side, but it _does_ diverge from the Chef API a bit (I wonder why they made that specific choice?).
So OK with leaving this if it's a pattern you'd like to continue for the remainder of the `/environment/...` API calls.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13/files#r6239087
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Override
> + public Iterable<? extends Node> execute(final ListeningExecutorService executor, String environmentName, Iterable<String> toGet) {
> + ListenableFuture<List<Node>> futures = allAsList(transform(toGet, new Function<String, ListenableFuture<Node>>() {
> + @Override
> + public ListenableFuture<Node> apply(final String input) {
> + return executor.submit(new Callable<Node>() {
> + @Override
> + public Node call() throws Exception {
> + return api.getNode(input);
> + }
> + });
> + }
> + }));
> +
> + logger.trace(String.format("getting nodes: %s", Joiner.on(',').join(toGet)));
Worth adding the name of the environment to the log?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13/files#r6238664
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Ignasi Barrera <no...@github.com>.
Closed #13.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Andrew Phillips <no...@github.com>.
> @@ -177,4 +177,7 @@
>
> @SinceApiVersion("0.10.0")
> Iterable<? extends Environment> listEnvironmentsNamed(Iterable<String> names);
> +
> + @SinceApiVersion("0.10.0")
> + Iterable<? extends Node> listEnvironmentNodes(String environmentName);
I'm guessing this is the [`/environments/NAME/nodes`](http://docs.opscode.com/api_chef_server_environments_node.html) call? Since it returns an iterable of Nodes, would it make more sense to call this "listNodesInEnvironment"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13/files#r6228562
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-chef #345](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-chef/345/) FAILURE
Looks like there's a problem with this pull request
[(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/13#issuecomment-24039973
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Noorul Islam K M <no...@github.com>.
> @@ -177,4 +177,7 @@
>
> @SinceApiVersion("0.10.0")
> Iterable<? extends Environment> listEnvironmentsNamed(Iterable<String> names);
> +
> + @SinceApiVersion("0.10.0")
> + Iterable<? extends Node> listEnvironmentNodes(String environmentName);
The api implementation is named listEnvironmentNodes. I am not sure whether we have to change it there also.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13/files#r6230473
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #35](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/35/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-24132977
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #30](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/30/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-24040004
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #38](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/38/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-24152458
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Andrew Phillips <no...@github.com>.
> @@ -177,4 +177,7 @@
>
> @SinceApiVersion("0.10.0")
> Iterable<? extends Environment> listEnvironmentsNamed(Iterable<String> names);
> +
> + @SinceApiVersion("0.10.0")
> + Iterable<? extends Node> listEnvironmentNodes(String environmentName);
> I meant in jclouds-chef
Ah, OK. I guess within jclouds-chef the naming should be consistent, yes.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13/files#r6232968
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #28](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/28/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-24039426
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #27](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/27/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-24039378
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-chef #342](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-chef/342/) FAILURE
Looks like there's a problem with this pull request
[(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/13#issuecomment-24039333
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -177,4 +177,7 @@
>
> @SinceApiVersion("0.10.0")
> Iterable<? extends Environment> listEnvironmentsNamed(Iterable<String> names);
> +
> + @SinceApiVersion("0.10.0")
> + Iterable<? extends Node> listEnvironmentNodes(String environmentName);
Perhaps I'd prefer to keep the current name. There will be a couple more api calls that will be added to work with environments (to get the cookbooks and roles), and having all those methods prefixed with "Environment" seems like a good way to group them. WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13/files#r6238746
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Andrew Phillips <no...@github.com>.
> @nacx What do you mean by squashing the commits?
Combining them into a single commit so the source code tree doesn't have to reflect the review process of this PR. See http://git-scm.com/book/en/Git-Tools-Rewriting-History for details.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-24148243
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Ignasi Barrera <no...@github.com>.
Merged: https://git-wip-us.apache.org/repos/asf?p=incubator-jclouds-chef.git;h=5733d69
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-24152854
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #37](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/37/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-24151808
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Noorul Islam K M <no...@github.com>.
> @@ -177,4 +177,7 @@
>
> @SinceApiVersion("0.10.0")
> Iterable<? extends Environment> listEnvironmentsNamed(Iterable<String> names);
> +
> + @SinceApiVersion("0.10.0")
> + Iterable<? extends Node> listEnvironmentNodes(String environmentName);
I meant in jclouds-chef
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13/files#r6232780
Re: [jclouds-chef] JCLOUDS-265: Add listEnvironmentNodes API in
ChefService (#13)
Posted by Ignasi Barrera <no...@github.com>.
Great @noorul! Could you squash the commits and rebase to the latest master version before I merge this? Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/13#issuecomment-24145851