You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2013/08/28 15:06:17 UTC
[jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Refactored the domain model to be immutable and addressed some inconsistences with the Chef Server API model.
Removed all HEAD methods, as they have been removed from newer versions of Chef. They were used to test the existance of a given resource, and with newer versions the only way to do that is via a GET operation.
Now all live tests are passing for Community Chef 0.10.8, 11.0.6 and Enterprise Chef.
You can merge this Pull Request by running:
git pull https://github.com/nacx/jclouds-chef jclouds-245/254
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-chef/pull/10
-- Commit Summary --
* JCLOUDS-245/JCLOUDS-254: Fix live tests
-- File Changes --
M README.md (2)
M compute/src/test/java/org/jclouds/chef/compute/ChefComputeServiceLiveTest.java (2)
M core/src/main/java/org/jclouds/chef/ChefApi.java (77)
M core/src/main/java/org/jclouds/chef/config/ChefParserModule.java (87)
M core/src/main/java/org/jclouds/chef/domain/Attribute.java (100)
M core/src/main/java/org/jclouds/chef/domain/BootstrapConfig.java (12)
M core/src/main/java/org/jclouds/chef/domain/ChecksumStatus.java (45)
M core/src/main/java/org/jclouds/chef/domain/Client.java (81)
M core/src/main/java/org/jclouds/chef/domain/CookbookDefinition.java (71)
M core/src/main/java/org/jclouds/chef/domain/CookbookVersion.java (197)
M core/src/main/java/org/jclouds/chef/domain/DatabagItem.java (2)
M core/src/main/java/org/jclouds/chef/domain/Environment.java (138)
M core/src/main/java/org/jclouds/chef/domain/Metadata.java (236)
M core/src/main/java/org/jclouds/chef/domain/Node.java (245)
M core/src/main/java/org/jclouds/chef/domain/Resource.java (70)
M core/src/main/java/org/jclouds/chef/domain/Role.java (120)
M core/src/main/java/org/jclouds/chef/domain/Sandbox.java (73)
M core/src/main/java/org/jclouds/chef/domain/SearchResult.java (6)
M core/src/main/java/org/jclouds/chef/domain/UploadSandbox.java (58)
M core/src/main/java/org/jclouds/chef/functions/ClientForGroup.java (7)
M core/src/main/java/org/jclouds/chef/strategy/internal/CleanupStaleNodesAndClientsImpl.java (2)
M core/src/main/java/org/jclouds/chef/strategy/internal/CreateNodeAndPopulateAutomaticAttributesImpl.java (19)
M core/src/main/java/org/jclouds/chef/strategy/internal/UpdateAutomaticAttributesOnNodeImpl.java (14)
M core/src/main/java/org/jclouds/chef/test/TransientChefApi.java (25)
M core/src/test/java/org/jclouds/chef/ChefApiTest.java (96)
M core/src/test/java/org/jclouds/chef/binders/BindHexEncodedMD5sToJsonPayloadTest.java (4)
A core/src/test/java/org/jclouds/chef/config/ChefParserModuleTest.java (95)
M core/src/test/java/org/jclouds/chef/functions/ParseClientFromJsonTest.java (4)
M core/src/test/java/org/jclouds/chef/functions/ParseCookbookDefinitionFromJsonv10Test.java (15)
M core/src/test/java/org/jclouds/chef/functions/ParseCookbookDefinitionListFromJsonv10Test.java (21)
M core/src/test/java/org/jclouds/chef/functions/ParseCookbookVersionFromJsonTest.java (82)
M core/src/test/java/org/jclouds/chef/functions/ParseNodeFromJsonTest.java (12)
M core/src/test/java/org/jclouds/chef/functions/ParseSandboxFromJsonTest.java (7)
M core/src/test/java/org/jclouds/chef/functions/ParseUploadSandboxFromJsonTest.java (26)
M core/src/test/java/org/jclouds/chef/functions/UriForResourceTest.java (2)
M core/src/test/java/org/jclouds/chef/internal/BaseChefApiLiveTest.java (144)
M core/src/test/java/org/jclouds/chef/strategy/internal/CreateNodeAndPopulateAutomaticAttributesImplLiveTest.java (2)
M core/src/test/java/org/jclouds/chef/strategy/internal/CreateNodeAndPopulateAutomaticAttributesImplTest.java (12)
M core/src/test/java/org/jclouds/chef/strategy/internal/UpdateAutomaticAttributesOnNodeImplLiveTest.java (4)
M core/src/test/java/org/jclouds/chef/strategy/internal/UpdateAutomaticAttributesOnNodeImplTest.java (17)
M core/src/test/java/org/jclouds/chef/test/TransientChefApiIntegrationTest.java (12)
M core/src/test/resources/apache-chef-demo-cookbook.json (21)
M core/src/test/resources/brew-cookbook.json (49)
M core/src/test/resources/client.json (9)
D core/src/test/resources/client.txt (7)
M core/src/test/resources/logback.xml (5)
M core/src/test/resources/mysql-cookbook.json (269)
D core/src/test/resources/newclient.txt (1)
M core/src/test/resources/node.json (12)
M core/src/test/resources/tomcat-cookbook.json (122)
M enterprise/src/main/java/org/jclouds/enterprisechef/EnterpriseChefApi.java (15)
M enterprise/src/main/java/org/jclouds/enterprisechef/domain/Group.java (118)
M enterprise/src/main/java/org/jclouds/enterprisechef/domain/User.java (123)
M enterprise/src/test/java/org/jclouds/enterprisechef/EnterpriseChefApiExpectTest.java (15)
M enterprise/src/test/java/org/jclouds/enterprisechef/EnterpriseChefApiLiveTest.java (20)
M enterprise/src/test/java/org/jclouds/enterprisechef/binders/BindGroupToUpdateRequestJsonPayloadTest.java (9)
M enterprise/src/test/java/org/jclouds/enterprisechef/binders/GroupNameTest.java (2)
-- Patch Links --
https://github.com/jclouds/jclouds-chef/pull/10.patch
https://github.com/jclouds/jclouds-chef/pull/10.diff
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> + this.groups.addAll(checkNotNull(groups, "groups"));
> + return this;
> + }
> +
> + public Builder user(String user) {
> + this.users.add(checkNotNull(user, "user"));
> + return this;
> + }
> +
> + public Builder users(Iterable<String> users) {
> + this.users.addAll(checkNotNull(users, "users"));
> + return this;
> + }
> +
> + public Group build() {
> + return new Group(name, checkNotNull(groupname, "groupame"), orgname, actors.build(), clients.build(), groups.build(), users.build());
Typo ("groupame"). Make `groupname` a required constructor property of the Builder?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6040220
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #16](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/16/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10#issuecomment-23412577
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
Nice! Some comments that keep repeating:
* can properties in objects with builders be `final`
* create a utility method for the `coll == null ? ImmutableX.of() : ImmutableX.copyOf(coll)` pattern?
* add messages to `assertX` checks
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10#issuecomment-23433001
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> }
>
> - public Version(URI url, String version) {
> + private URI url;
> + private String version;
Can be `final`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039791
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> + @SerializedName("override")
> + private Map<String, JsonBall> overrideAttributes;
> + @SerializedName("default")
> + private Map<String, JsonBall> defaultAttributes;
> + @SerializedName("automatic")
> + private Map<String, JsonBall> automaticAttributes;
> + @SerializedName("run_list")
> + private List<String> runList;
> + @SerializedName("chef_environment")
> + private String environment;
> +
> + // internal
> + @SerializedName("json_class")
> + private String _jsonClass = "Chef::Node";
> + @SerializedName("chef_type")
> + private String _chefType = "node";
Can these be `final`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039915
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -211,6 +220,83 @@ protected String toString(DatabagItem value) {
> private String id;
> }
>
> + // The NullFilteringTypeAdapterFactories.MapTypeAdapter class is final. Do
> + // the same logic here
> + private static final class KeepLastRepeatedKeyMapTypeAdapter<K, V> extends TypeAdapter<Map<K, V>> {
The class has also an explicit private default constructor, so it seems it is not intended to have it extended (don't know why, but I'd prefer to not change that just for this particular case).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6047402
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> +
> + @Override
> + public int hashCode() {
> + return Objects.hashCode(key, value);
> + }
> +
> + @Override
> + public boolean equals(Object obj) {
> + if (this == obj)
> + return true;
> + if (obj == null || getClass() != obj.getClass())
> + return false;
> + KeyValue that = KeyValue.class.cast(obj);
> + return equal(this.key, that.key) && equal(this.value, that.value);
> + }
> + }
Can't use `Map.Entry` or similar here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6040051
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> + public static class Builder {
> + private URI url;
> + private boolean needsUpload;
> +
> + public Builder url(URI url) {
> + this.url = checkNotNull(url, "url");
> + return this;
> + }
> +
> + public Builder needsUpload(boolean needsUpload) {
> + this.needsUpload = needsUpload;
> + return this;
> + }
> +
> + public ChecksumStatus build() {
> + return new ChecksumStatus(url, needsUpload);
Will this work if `url` is null?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039700
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> this.required = required;
> this.calculated = calculated;
> - Iterables.addAll(this.choice, choice);
> + this.choice = choice == null ? ImmutableSet.<String> of() : ImmutableSet.copyOf(choice);
Any specific reason for this logic change?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039671
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Ignasi Barrera <no...@github.com>.
Thanks for taking your time to review this @demobox!
I've addressed your comments and properly implemented the immutable model. Thx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10#issuecomment-23450233
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> @@ -76,11 +128,11 @@ public String getDescription() {
> }
>
> public Map<String, JsonBall> getOverride() {
Rename getter to match property name?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6040008
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> + this.recommendations.put(checkNotNull(key, "key"), checkNotNull(value, "value"));
> + return this;
> + }
> +
> + public Builder recommendations(Map<String, String> recommendations) {
> + this.recommendations.putAll(checkNotNull(recommendations, "recommendations"));
> + return this;
> + }
> +
> + public Metadata build() {
> + return new Metadata(license, maintainer, suggestions.build(), dependencies.build(), maintainerEmail,
> + conflicting.build(), description, providing.build(), platforms.build(), version, recipes.build(),
> + replacing.build(), name, groupings.build(), longDescription, attributes.build(), recommendations.build());
> + }
> +
> + }
Fff...I'm wondering...is there no builder code generation library out there..? ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039887
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> @@ -150,8 +202,8 @@ public boolean equals(Object obj) {
>
> @Override
> public String toString() {
> - return "[name=" + name + ", description=" + description + ", defaultA=" + defaultA + ", override=" + override
> - + ", runList=" + runList + "]";
> + return "Role [name=" + name + ", description=" + description + ", defaultA=" + defaultAttributes + ", override="
> + + overrideAttributes + ", runList=" + runList + "]";
Rename `defaultA` etc. in the string?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039964
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> @SerializedName("cookbook_versions")
> - private Map<String, String> cookbookVersions = Maps.newLinkedHashMap();
> + private Map<String, String> cookbookVersions;
> +
Make `final`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039857
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> this.cookbookName = cookbookName;
> - Iterables.addAll(this.resources, resources);
> - Iterables.addAll(this.templates, templates);
> - Iterables.addAll(this.libraries, libraries);
> + this.resources = resources == null ? ImmutableSet.<Resource> of() : ImmutableSet.copyOf(resources);
Factor this pattern out into a method `copyOfOrEmpty`, or something like that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039832
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> + public Builder checksum(byte[] checksum) {
> + this.checksum = checkNotNull(checksum, "checksum");
> + return this;
> + }
> +
> + public Builder path(String path) {
> + this.path = checkNotNull(path, "path");
> + return this;
> + }
> +
> + public Builder specificity(String specificity) {
> + this.specificity = checkNotNull(specificity, "specificity");
> + return this;
> + }
> +
> + public Builder fromPayload(FilePayload payload) {
`checkNotNull(payload)`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039939
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> - for (String version : api.getVersionsOfCookbook(cookbook)) {
> - CookbookVersion cookbookO = api.getCookbook(cookbook, version);
> - for (Resource resource : ImmutableList.<Resource> builder().addAll(cookbookO.getDefinitions())
> - .addAll(cookbookO.getFiles()).addAll(cookbookO.getLibraries()).addAll(cookbookO.getSuppliers())
> - .addAll(cookbookO.getRecipes()).addAll(cookbookO.getResources()).addAll(cookbookO.getRootFiles())
> - .addAll(cookbookO.getTemplates()).build()) {
> - try {
> - InputStream stream = api.getResourceContents(resource);
> - byte[] md5 = asByteSource(stream).hash(md5()).asBytes();
> - assertEquals(md5, resource.getChecksum());
> - } catch (NullPointerException e) {
> - assert false : "resource not found: " + resource;
> - }
> + for (String cookbookName : cookbookNames) {
> + Set<String> versions = api.getVersionsOfCookbook(cookbookName);
> + assertFalse(versions.isEmpty());
Add message?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6040121
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #19](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/19/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10#issuecomment-23450368
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> @@ -37,9 +88,15 @@
> private String name;
> private boolean validator;
Can these be `final`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039731
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Ignasi Barrera <no...@github.com>.
> + out.name(String.valueOf(element.getKey()));
> + valueAdapter.write(out, element.getValue());
> + }
> + out.endObject();
> + }
> +
> + public Map<K, V> read(JsonReader in) throws IOException {
> + Map<K, V> result = Maps.newHashMap();
> + in.beginObject();
> + while (in.hasNext()) {
> + JsonReaderInternalAccess.INSTANCE.promoteNameToValue(in);
> + K name = keyAdapter.read(in);
> + V value = valueAdapter.read(in);
> + if (value != null) {
> + // If there are repeated keys, only keep the last one
> + result.remove(name);
Sure. It was just legacy because in a previous iteration the `result` variable was an `ImmutableMap.builder` which allowed multiple puts with the same key, failing in the build() method. Will remove this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6036104
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> }
>
> - public CookbookDefinition(URI url, Set<Version> versions) {
> + private URI url;
> + private Set<Version> versions;
Can be `final`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039784
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> + out.name(String.valueOf(element.getKey()));
> + valueAdapter.write(out, element.getValue());
> + }
> + out.endObject();
> + }
> +
> + public Map<K, V> read(JsonReader in) throws IOException {
> + Map<K, V> result = Maps.newHashMap();
> + in.beginObject();
> + while (in.hasNext()) {
> + JsonReaderInternalAccess.INSTANCE.promoteNameToValue(in);
> + K name = keyAdapter.read(in);
> + V value = valueAdapter.read(in);
> + if (value != null) {
> + // If there are repeated keys, only keep the last one
> + result.remove(name);
Do we need this call? Doesn't `put` overwrite?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6035960
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + @Test(dependsOnMethods = "testListCookbookVersionsWithChefService")
> + public void testDownloadCookbooks() throws Exception {
> + Iterable<? extends CookbookVersion> cookbooks = chefService.listCookbookVersions();
> + for (CookbookVersion cookbook : cookbooks) {
> + for (Resource resource : ImmutableList.<Resource> builder().addAll(cookbook.getDefinitions())
> + .addAll(cookbook.getFiles()).addAll(cookbook.getLibraries()).addAll(cookbook.getSuppliers())
> + .addAll(cookbook.getRecipes()).addAll(cookbook.getResources()).addAll(cookbook.getRootFiles())
> + .addAll(cookbook.getTemplates()).build()) {
> + try {
> + InputStream stream = api.getResourceContents(resource);
> + byte[] md5 = asByteSource(stream).hash(md5()).asBytes();
> + assertEquals(md5, resource.getChecksum());
> + } catch (NullPointerException e) {
> + fail("resource not found: " + resource);
Write instead as
```
assertNotNull(resource, "...")
assertEquals(...)
```
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6040157
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> @@ -70,7 +68,6 @@ protected void configure() {
> }));
> }
>
> - @Test(enabled = false)
Yay! ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6040082
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> +
> + public Builder url(URI url) {
> + this.url = checkNotNull(url, "url");
> + return this;
> + }
> +
> + public Builder needsUpload(boolean needsUpload) {
> + this.needsUpload = needsUpload;
> + return this;
> + }
> +
> + public ChecksumStatus build() {
> + return new ChecksumStatus(url, needsUpload);
> + }
> + }
> +
> private URI url;
> @SerializedName("needs_upload")
> private boolean needsUpload;
Make both of these `final`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039715
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-chef #309](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-chef/309/) 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/10#issuecomment-23450072
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> - .addAll(cookbookO.getRecipes()).addAll(cookbookO.getResources()).addAll(cookbookO.getRootFiles())
> - .addAll(cookbookO.getTemplates()).build()) {
> - try {
> - InputStream stream = api.getResourceContents(resource);
> - byte[] md5 = asByteSource(stream).hash(md5()).asBytes();
> - assertEquals(md5, resource.getChecksum());
> - } catch (NullPointerException e) {
> - assert false : "resource not found: " + resource;
> - }
> + for (String cookbookName : cookbookNames) {
> + Set<String> versions = api.getVersionsOfCookbook(cookbookName);
> + assertFalse(versions.isEmpty());
> +
> + for (String version : api.getVersionsOfCookbook(cookbookName)) {
> + CookbookVersion cookbook = api.getCookbook(cookbookName, version);
> + assertNotNull(cookbook);
Add message?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6040128
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #21](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/21/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10#issuecomment-23499160
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Ignasi Barrera <no...@github.com>.
@demobox If you have a minute could you have a final look at this and see if it can be merged? :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10#issuecomment-23571211
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> @@ -211,6 +220,83 @@ protected String toString(DatabagItem value) {
> private String id;
> }
>
> + // The NullFilteringTypeAdapterFactories.MapTypeAdapter class is final. Do
> + // the same logic here
> + private static final class KeepLastRepeatedKeyMapTypeAdapter<K, V> extends TypeAdapter<Map<K, V>> {
Make this a subclass of the factory, and perhaps move the factory into a different class? Quite a big chunk of code here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6036000
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> return false;
> return true;
> }
>
> + @Override
> + public String toString() {
> + return "Node [name=" + name + ", runList=" + runList + ", normal=" + normalAttributes + ", default="
> + + defaultAttributes + ", override=" + overrideAttributes + ", chefEnvironment=" + environment
> + + ", automatic=" + automaticAttributes + "]";
Also rename `override` etc. in the string?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039928
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #18](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/18/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10#issuecomment-23450548
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> @@ -150,6 +172,7 @@ public void testListCookbooks() throws Exception {
> @Test(dependsOnMethods = "testCreateNewCookbook")
> public void testUpdateCookbook() throws Exception {
> CookbookVersion cookbook = api.getCookbook(PREFIX, "0.0.0");
> + assertNotNull(cookbook);
Add message?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6040168
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> @@ -294,9 +298,9 @@ public void testUpdateDatabagItem() throws Exception {
> public void testListSearchIndexes() throws Exception {
> Set<String> indexes = api.listSearchIndexes();
> assertNotNull(indexes);
> - assert indexes.contains("node") : indexes;
> - assert indexes.contains("client") : indexes;
> - assert indexes.contains("role") : indexes;
> + assertTrue(indexes.contains("node"));
> + assertTrue(indexes.contains("client"));
> + assertTrue(indexes.contains("role"));
Add messages?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6040175
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> @@ -124,14 +175,10 @@ public boolean equals(Object obj) {
> return true;
> }
>
> - public Client(X509Certificate certificate, String orgname, String clientname, String name, boolean isValidator,
> - @Nullable PrivateKey privateKey) {
> - this.certificate = certificate;
> - this.orgname = orgname;
> - this.clientname = clientname;
> - this.name = name;
> - this.validator = isValidator;
> - this.privateKey = privateKey;
> + @Override
> + public String toString() {
> + return "Client [name=" + name + ", clientname=" + clientname + ", orgname=" + orgname + ", isValidator="
> + + validator + ", certificate=" + certificate + ", privateKey=" + (privateKey != null) + "]";
`privateKey=` is confusing? Should that rather be something like `privateKey " + (privateKey == null) ? "not " : "" + "set"`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039775
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #17](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/17/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10#issuecomment-23450151
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-chef #311](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-chef/311/) 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/10#issuecomment-23450545
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> + this.recipes.addAll(checkNotNull(recipes, "recipes"));
> + return this;
> + }
> +
> + public Builder rootFile(Resource rootFile) {
> + this.rootFiles.add(checkNotNull(rootFile, "rootFile"));
> + return this;
> + }
> +
> + public Builder rootFiles(Iterable<Resource> rootFiles) {
> + this.rootFiles.addAll(checkNotNull(rootFiles, "rootFiles"));
> + return this;
> + }
> +
> + public CookbookVersion build() {
> + return new CookbookVersion(checkNotNull(cookbookName, "name") + "-" + checkNotNull(version, "version"),
If these have to be set, perhaps make them constructor arguments of the `Builder`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039808
Re: [jclouds-chef] JCLOUDS-245/JCLOUDS-254: Fix live tests (#10)
Posted by Andrew Phillips <no...@github.com>.
> this.name = name;
> - this.groupings.putAll(groupings);
> + this.groupings = groupings == null ? ImmutableMap.<String, String> of() : ImmutableMap.copyOf(groupings);
See comments above about this pattern
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/10/files#r6039894