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/10/29 08:15:39 UTC
[jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
...PI
You can merge this Pull Request by running:
git pull https://github.com/noorul/jclouds-chef jclouds-274
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-chef/pull/32
-- Commit Summary --
* JCLOUDS-274 Implement listCookbookVersionsInEnvironment in ChefService API
-- File Changes --
A core/src/main/java/org/jclouds/chef/strategy/ListCookbookVersionsInEnvironment.java (61)
A core/src/main/java/org/jclouds/chef/strategy/internal/ListCookbookVersionsInEnvironmentImpl.java (171)
A core/src/test/java/org/jclouds/chef/strategy/internal/ListCookbookVersionsInEnvironmentImplLiveTest.java (203)
-- Patch Links --
https://github.com/jclouds/jclouds-chef/pull/32.patch
https://github.com/jclouds/jclouds-chef/pull/32.diff
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Andrew Phillips <no...@github.com>.
> +import org.jclouds.chef.domain.CookbookVersion;
> +import org.jclouds.chef.strategy.internal.ListCookbookVersionsInEnvironmentImpl;
> +
> +import com.google.common.base.Predicate;
> +import com.google.common.util.concurrent.ListeningExecutorService;
> +import com.google.inject.ImplementedBy;
> +
> +/**
> + *
> + *
> + * @author Noorul Islam K M
> + */
> +@ImplementedBy(ListCookbookVersionsInEnvironmentImpl.class)
> +public interface ListCookbookVersionsInEnvironment {
> +
> + public Iterable<? extends CookbookVersion> execute(String environmentName);
Remove `public` from all of these - interface methods are public by definition.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32/files#r7279962
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-chef #570](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-chef/570/) 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/32#issuecomment-27679029
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-chef #542](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-chef/542/) 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/32#issuecomment-27284323
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #84](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/84/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27383762
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Ignasi Barrera <no...@github.com>.
Looks good to me too.
@noorul, could you change the `fail` method, and move the private helper methods to the bottom of the class, and leave the setup/teardown and the actual tests first?
Once that is done I'll merge the PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27675830
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-chef #548](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-chef/548/) 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/32#issuecomment-27408731
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Noorul Islam K M <no...@github.com>.
I have to written unit tests since we are going to migrate to new framework. Live tests covers everything.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27282507
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Andrew Phillips <no...@github.com>.
> + * Lists the details of all existing cookbooks in an environment.
> + *
> + * @param environmentName The environment name.
> + * @return The details of all existing cookbooks in an environment.
> + */
> + Iterable<? extends CookbookVersion> listCookbookVersionsInEnvironment(String environmentName);
> +
> + /**
> + * Lists the details of all existing cookbooks in an environment
> + * limiting number of versions.
> + *
> + * @param environmentName The environment name.
> + * @param numVersions Number of versions to limit.
> + * @return The details of all existing cookbooks in environment.
> + */
> + Iterable<? extends CookbookVersion> listCookbookVersionsInEnvironment(String environmentName, String numVersions);
Why a string rather than some numeric type for `numVersions`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32/files#r7279866
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Ignasi Barrera <no...@github.com>.
> + // Request an upload site for this file
> + UploadSandbox site = api.createUploadSandboxForChecksums(ImmutableSet.of(md5));
> + assertTrue(site.getChecksums().containsKey(md5), md5 + " not in " + site.getChecksums());
> +
> + try {
> + // Upload the file contents, if still not uploaded
> + ChecksumStatus status = site.getChecksums().get(md5);
> + if (status.needsUpload()) {
> + api.uploadContent(status.getUrl(), content);
> + }
> + Sandbox sandbox = api.commitSandbox(site.getSandboxId(), true);
> + assertTrue(sandbox.isCompleted(), "Sandbox should be completed after uploading");
> + } catch (RuntimeException e) {
> + api.commitSandbox(site.getSandboxId(), false);
> + fail("Could not upload content");
> + return null;
The `fail` method will already throw an exception, so there is no need to return null here. This is unreachable code.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32/files#r7272837
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -238,6 +242,16 @@ public void deleteAllClientsInList(Iterable<String> names) {
> }
>
> @Override
> + public Iterable<? extends CookbookVersion> listCookbookVersionsInEnvironment(String environmentName) {
> + return listCookbookVersionsInEnvironment.execute(environmentName);
> + }
> +
> + @Override
> + public Iterable<? extends CookbookVersion> listCookbookVersionsInEnvironment(String environmentName, String numVersions) {
> + return listCookbookVersionsInEnvironment.execute(environmentName, numVersions);
Use numeric type rather than String for `numVersions`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32/files#r7279880
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Noorul Islam K M <no...@github.com>.
> @@ -187,6 +187,24 @@
> Iterable<? extends CookbookVersion> listCookbookVersions();
>
> /**
> + * Lists the details of all existing cookbooks in an environment.
> + *
> + * @param environmentName The environment name.
> + * @return The details of all existing cookbooks in an environment.
> + */
> + Iterable<? extends CookbookVersion> listCookbookVersionsInEnvironment(String environmentName);
> +
> + /**
> + * Lists the details of all existing cookbooks in an environment
> + * limiting number of versions.
latest version. Documented.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32/files#r7308782
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Andrew Phillips <no...@github.com>.
> +
> + public Iterable<? extends CookbookVersion> execute(String environmentName, String numVersions, Iterable<String> cookbookNames);
> +
> + public Iterable<? extends CookbookVersion> execute(ListeningExecutorService executor, String environmentName);
> +
> + public Iterable<? extends CookbookVersion> execute(ListeningExecutorService executor, String environmentName, String numVersions);
> +
> + public Iterable<? extends CookbookVersion> execute(ListeningExecutorService executor, String environmentName,
> + Predicate<CookbookDefinition> cookbookNameSelector);
> +
> + public Iterable<? extends CookbookVersion> execute(ListeningExecutorService executor, String environmentName, String numVersions,
> + Predicate<CookbookDefinition> cookbookNameSelector);
> +
> + public Iterable<? extends CookbookVersion> execute(ListeningExecutorService executor, String environmentName, Iterable<String> cookbookNames);
> +
> + public Iterable<? extends CookbookVersion> execute(ListeningExecutorService executor, String environmentName, String numVersions, Iterable<String> cookbookNames);
Why all these different variants if we're only exposing two to the main API?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32/files#r7279917
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #81](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/81/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27282601
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #89](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/89/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27461765
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Andrew Phillips <no...@github.com>.
> My vote is to refactor all similar strategies in a different pull request and remove all methods that use a predicate or
> a "to get" list (in other words, remove all methods that are not exposed in the ChefService, as I'm pretty sure
> nobody is using them right now and provide very little value).
Thanks for explaining more of the background, @nacx! +1 on doing this in a separate PR...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27541063
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #86](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/86/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27408450
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Andrew Phillips <no...@github.com>.
General question: why are there so many different methods with similar signatures in `ListCookbookVersionsInEnvironment` if only two methods are exposed in `ChefService`. I would prefer to implement the minimum amount of methods now and add more options as we expose them to users.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27306303
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Andrew Phillips <no...@github.com>.
> Can we merge this?
Looks OK to me. @nacx: Good to go for you, too?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27625336
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Andrew Phillips <no...@github.com>.
> + * limitations under the License.
> + */
> +package org.jclouds.chef.strategy;
> +
> +import org.jclouds.chef.domain.CookbookDefinition;
> +import org.jclouds.chef.domain.CookbookVersion;
> +import org.jclouds.chef.strategy.internal.ListCookbookVersionsInEnvironmentImpl;
> +
> +import com.google.common.base.Predicate;
> +import com.google.common.util.concurrent.ListeningExecutorService;
> +import com.google.inject.ImplementedBy;
> +
> +/**
> + *
> + *
> + * @author Noorul Islam K M
[minor] remove blank comment line
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32/files#r7279899
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Ignasi Barrera <no...@github.com>.
>General question: why are there so many different methods with similar signatures in ListCookbookVersionsInEnvironment if only two methods are exposed in ChefService. I would prefer to implement the minimum amount of methods now and add more options as we expose them to users.
Personally, I think this is a pattern we have been copying between the strategy classes, but I don't find it that useful.
The Chef api does not return the entire objects when calling the "list" methods; it only returns the name, the url, and a few more fields. The motivation behind some of the existing strategy classes is to provide a way to get the list of objects with all the details populated, and do it concurrently. Those strategies also contain several convenience methods to apply filtering, etc. Those strategies have some of their methods exposed in the ChefService, so their functionality can be easily accessed.
That said, I think there are many premature "sugars" that are not that useful (users can implement them very easily) and that we could remove. Personally I think all methods receiving a predicate as a parameter, and a list of elements "to get" should be removed, as that can be done afterwards by the callers.
My vote is to refactor all similar strategies in a different pull request and remove all methods that use a predicate or a "to get" list (in other words, remove all methods that are not exposed in the ChefService, as I'm pretty sure nobody is using them right now and provide very little value).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27431502
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Noorul Islam K M <no...@github.com>.
> + * Lists the details of all existing cookbooks in an environment.
> + *
> + * @param environmentName The environment name.
> + * @return The details of all existing cookbooks in an environment.
> + */
> + Iterable<? extends CookbookVersion> listCookbookVersionsInEnvironment(String environmentName);
> +
> + /**
> + * Lists the details of all existing cookbooks in an environment
> + * limiting number of versions.
> + *
> + * @param environmentName The environment name.
> + * @param numVersions Number of versions to limit.
> + * @return The details of all existing cookbooks in environment.
> + */
> + Iterable<? extends CookbookVersion> listCookbookVersionsInEnvironment(String environmentName, String numVersions);
This can take a value of 'all'. Documented it now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32/files#r7308777
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Noorul Islam K M <no...@github.com>.
Since we already implemented, I think we should expose all of them. @nacx any thoughts on this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27383464
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Noorul Islam K M <no...@github.com>.
Can we merge this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27620492
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #94](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/94/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27679152
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Ignasi Barrera <no...@github.com>.
Merged. All live tests passing in Chef 10, Chef 11 and Enterprise Chef. Great @noorul !
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27765815
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #82](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/82/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27284291
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -187,6 +187,24 @@
> Iterable<? extends CookbookVersion> listCookbookVersions();
>
> /**
> + * Lists the details of all existing cookbooks in an environment.
> + *
> + * @param environmentName The environment name.
> + * @return The details of all existing cookbooks in an environment.
> + */
> + Iterable<? extends CookbookVersion> listCookbookVersionsInEnvironment(String environmentName);
> +
> + /**
> + * Lists the details of all existing cookbooks in an environment
> + * limiting number of versions.
Which versions do you get? The most recent ones? Arbitrary?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32/files#r7279848
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by Ignasi Barrera <no...@github.com>.
> + .rootFile(Resource.builder().fromPayload(v1content).build()) //
> + .build();
> +
> + // upload the cookbook to the remote server
> + api.updateCookbook(cookbookName, "1.0.0", cookbook);
> + }
> +
> + @Override
> + protected void initialize() {
> + super.initialize();
> +
> + try {
> + createCookbooksWithMultipleVersions(PREFIX);
> + createCookbooksWithMultipleVersions(PREFIX + 1);
> + } catch (Exception e) {
> + fail(String.format("Could not create cookbooks: %s", e));
Use better: `fail("Could not create cookbooks", e)`, to let the 'fail' method properly report the exception.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32/files#r7396782
Re: [jclouds-chef] JCLOUDS-274 Implement
listCookbookVersionsInEnvironment in ChefService A... (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #83](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/83/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/32#issuecomment-27284445