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/09/10 23:00:14 UTC
[jclouds-chef] JCLOUDS-253: Properly parse Chef Server version (#18)
Properly parse the configured api version so the appropriate cookbook response parser is used.
You can merge this Pull Request by running:
git pull https://github.com/nacx/jclouds-chef 253-parseversion
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-chef/pull/18
-- Commit Summary --
* JCLOUDS-253: Properly parse Chef Server version
-- File Changes --
M core/src/main/java/org/jclouds/chef/config/ChefParserModule.java (29)
A core/src/main/java/org/jclouds/chef/suppliers/ChefVersionSupplier.java (76)
A core/src/test/java/org/jclouds/chef/suppliers/ChefVersionSupplierTest.java (47)
-- Patch Links --
https://github.com/jclouds/jclouds-chef/pull/18.patch
https://github.com/jclouds/jclouds-chef/pull/18.diff
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by Andrew Phillips <no...@github.com>.
+1 - looks good to me! Squash-n-merge time!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18#issuecomment-24382430
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #43](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/43/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18#issuecomment-24199423
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #42](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/42/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18#issuecomment-24194563
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by Ignasi Barrera <no...@github.com>.
> + public static final Integer DEFAULT_VERSION = 10;
> +
> + @Resource
> + @Named(ChefProperties.CHEF_LOGGER)
> + private Logger logger = Logger.NULL;
> +
> + /** The configured version of the Chef Server API. */
> + private final String apiVersion;
> +
> + @Inject
> + ChefVersionSupplier(@ApiVersion String apiVersion) {
> + this.apiVersion = checkNotNull(apiVersion, "apiVersion must not be null");
> + }
> +
> + @Override
> + public Integer get() {
This method is the one from the `Supplier` interface, so it must return the object instead of the primitive type.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18/files#r6280258
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by Andrew Phillips <no...@github.com>.
> + public static final Integer DEFAULT_VERSION = 10;
> +
> + @Resource
> + @Named(ChefProperties.CHEF_LOGGER)
> + private Logger logger = Logger.NULL;
> +
> + /** The configured version of the Chef Server API. */
> + private final String apiVersion;
> +
> + @Inject
> + ChefVersionSupplier(@ApiVersion String apiVersion) {
> + this.apiVersion = checkNotNull(apiVersion, "apiVersion must not be null");
> + }
> +
> + @Override
> + public Integer get() {
`int`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18/files#r6279887
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by Ignasi Barrera <no...@github.com>.
@demobox Improved the regular expression and implemented it as you suggested; it looks cleaner. The [test class](https://github.com/nacx/jclouds-chef/blob/fb8c7f1c38bc11cd7c758f5042244320ea9e60e1/core/src/test/java/org/jclouds/chef/suppliers/ChefVersionSupplierTest.java) shows how the version number is retrieved.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18#issuecomment-24350898
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by Andrew Phillips <no...@github.com>.
> @@ -254,7 +252,8 @@ public void write(JsonWriter out, Map<K, V> value) throws IOException {
> K name = keyAdapter.read(in);
> V value = valueAdapter.read(in);
> if (value != null) {
> - // If there are repeated keys, overwrite them to only keep the last one
> + // If there are repeated keys, overwrite them to only keep the
> + // last one
Any reason for this line break?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18/files#r6279840
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by Andrew Phillips <no...@github.com>.
> +
> + @Resource
> + @Named(ChefProperties.CHEF_LOGGER)
> + private Logger logger = Logger.NULL;
> +
> + /** The configured version of the Chef Server API. */
> + private final String apiVersion;
> +
> + @Inject
> + ChefVersionSupplier(@ApiVersion String apiVersion) {
> + this.apiVersion = checkNotNull(apiVersion, "apiVersion must not be null");
> + }
> +
> + @Override
> + public Integer get() {
> + Pattern versionPattern = Pattern.compile("(\\d+)\\.(\\d+)(\\.\\d+)*");
> Tried something like "0\\.(\\d+)|(\\d+)\\.\\d+", and although it works, the matched group is 1 or 2
Wow, learned something new there. So the indexing is relative to the *regex*, not the *match*? If the second one matches, what is `group(1)` returning??
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18/files#r6308956
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by Andrew Phillips <no...@github.com>.
> +
> + @Resource
> + @Named(ChefProperties.CHEF_LOGGER)
> + private Logger logger = Logger.NULL;
> +
> + /** The configured version of the Chef Server API. */
> + private final String apiVersion;
> +
> + @Inject
> + ChefVersionSupplier(@ApiVersion String apiVersion) {
> + this.apiVersion = checkNotNull(apiVersion, "apiVersion must not be null");
> + }
> +
> + @Override
> + public Integer get() {
> + Pattern versionPattern = Pattern.compile("(\\d+)\\.(\\d+)(\\.\\d+)*");
Indeed:
public static void main (String[] args) throws java.lang.Exception
{
Pattern p = Pattern.compile("(?:([1-9]\\d*)\\.\\d+)|(?:0\\.(\\d+))");
Matcher m = p.matcher("0.1");
System.out.println(m.groupCount());
System.out.println(m.group(1));
System.out.println(m.group(2));
m = p.matcher("1.0");
System.out.println(m.groupCount());
System.out.println(m.group(1));
System.out.println(m.group(2));
}
Output;
```
2
null
1
2
1
null
```
How about this and then `return Objects.firstNonNull(m.group(1), m.group(2));`? (from [Guava](http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/base/Objects.html#firstNonNull\(T, T\)))
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18/files#r6309112
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by Andrew Phillips <no...@github.com>.
> + public static final Integer DEFAULT_VERSION = 10;
> +
> + @Resource
> + @Named(ChefProperties.CHEF_LOGGER)
> + private Logger logger = Logger.NULL;
> +
> + /** The configured version of the Chef Server API. */
> + private final String apiVersion;
> +
> + @Inject
> + ChefVersionSupplier(@ApiVersion String apiVersion) {
> + this.apiVersion = checkNotNull(apiVersion, "apiVersion must not be null");
> + }
> +
> + @Override
> + public Integer get() {
> This method is the one from the Supplier interface, so it must return the object instead of the primitive type.
Erm...yes, indeed. Doh! ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18/files#r6281078
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-chef-pull-requests #44](https://jclouds.ci.cloudbees.com/job/jclouds-chef-pull-requests/44/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18#issuecomment-24351168
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by Andrew Phillips <no...@github.com>.
> +
> + @Resource
> + @Named(ChefProperties.CHEF_LOGGER)
> + private Logger logger = Logger.NULL;
> +
> + /** The configured version of the Chef Server API. */
> + private final String apiVersion;
> +
> + @Inject
> + ChefVersionSupplier(@ApiVersion String apiVersion) {
> + this.apiVersion = checkNotNull(apiVersion, "apiVersion must not be null");
> + }
> +
> + @Override
> + public Integer get() {
> + Pattern versionPattern = Pattern.compile("(\\d+)\\.(\\d+)(\\.\\d+)*");
If we're only interested in the major version, how about a regex for "0.(x)" or "(x).y" and then simply always return matching group 1?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18/files#r6281155
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds-chef #373](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-chef/373/) 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/18#issuecomment-24351010
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Resource
> + @Named(ChefProperties.CHEF_LOGGER)
> + private Logger logger = Logger.NULL;
> +
> + /** The configured version of the Chef Server API. */
> + private final String apiVersion;
> +
> + @Inject
> + ChefVersionSupplier(@ApiVersion String apiVersion) {
> + this.apiVersion = checkNotNull(apiVersion, "apiVersion must not be null");
> + }
> +
> + @Override
> + public Integer get() {
> + Pattern versionPattern = Pattern.compile("(\\d+)\\.(\\d+)(\\.\\d+)*");
Tried something like `"0\\.(\\d+)|(\\d+)\\.\\d+"`, and although it works, the matched group is 1 or 2, depending on which part of the regexp matches, so we can't always return group 1. An alternative would be to use two patterns and don't use the OR inside the regexp, but in that case I think the current implementation would be better. Thoughts?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18/files#r6307877
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -254,7 +252,8 @@ public void write(JsonWriter out, Map<K, V> value) throws IOException {
> K name = keyAdapter.read(in);
> V value = valueAdapter.read(in);
> if (value != null) {
> - // If there are repeated keys, overwrite them to only keep the last one
> + // If there are repeated keys, overwrite them to only keep the
> + // last one
Nope. Will remove it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18/files#r6280238
Re: [jclouds-chef] JCLOUDS-253: Properly parse Chef Server version
(#18)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Resource
> + @Named(ChefProperties.CHEF_LOGGER)
> + private Logger logger = Logger.NULL;
> +
> + /** The configured version of the Chef Server API. */
> + private final String apiVersion;
> +
> + @Inject
> + ChefVersionSupplier(@ApiVersion String apiVersion) {
> + this.apiVersion = checkNotNull(apiVersion, "apiVersion must not be null");
> + }
> +
> + @Override
> + public Integer get() {
> + Pattern versionPattern = Pattern.compile("(\\d+)\\.(\\d+)(\\.\\d+)*");
Cleaner :) Will change!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-chef/pull/18/files#r6281196