You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Nikolay <no...@github.com> on 2014/09/08 19:22:05 UTC
[jclouds] Throw sane error if one of security groups is null (#515)
You can merge this Pull Request by running:
git pull https://github.com/chemikadze/jclouds fix-nova-secgroup-npe
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/515
-- Commit Summary --
* Throw sane error if one of security groups is null
-- File Changes --
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/options/NovaTemplateOptions.java (2)
M compute/src/main/java/org/jclouds/compute/options/TemplateOptions.java (4)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/515.patch
https://github.com/jclouds/jclouds/pull/515.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/515
Re: [jclouds] Throw sane error if one of security groups is null
(#515)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds #1613](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1613/) 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/pull/515#issuecomment-54863308
Re: [jclouds] Throw sane error if one of security groups is null
(#515)
Posted by Andrew Phillips <no...@github.com>.
> @@ -179,6 +179,8 @@ public NovaTemplateOptions keyPairName(String keyPairName) {
> */
> @Deprecated
> public NovaTemplateOptions securityGroupNames(String... securityGroupNames) {
> + for (String groupName : checkNotNull(securityGroupNames, "securityGroupNames"))
> + checkNotNull(emptyToNull(groupName), "all security groups must be non-empty");
As @jdaggett said: please add `{ ... }`. Also:
* why are **empty** group names not allowed?
* we can remove the `checkNotNull` in the next line
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/515/files#r17993484
Re: [jclouds] Throw sane error if one of security groups is null
(#515)
Posted by Everett Toews <no...@github.com>.
Closed #515.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/515#event-272760104
Re: [jclouds] Throw sane error if one of security groups is null
(#515)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1157](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1157/) UNSTABLE
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/pull/515#issuecomment-54865315
Re: [jclouds] Throw sane error if one of security groups is null
(#515)
Posted by Jeremy Daggett <no...@github.com>.
Thanks for the contribution @chemikadze!
[minor] Small style issue, please put a block around the for loops `{` `}`. Also, if you could please add test cases in `NovaTemplateOptionsTest`, that would be great!
Currently, the core `TemplateOptions` class does not have tests for security groups, and I just submitted [JCLOUDS-713](https://issues.apache.org/jira/browse/JCLOUDS-713) to add them. /cc @abayer
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/515#issuecomment-55015039
Re: [jclouds] Throw sane error if one of security groups is null
(#515)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-6 #68](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/68/) 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/pull/515#issuecomment-54859851
Re: [jclouds] Throw sane error if one of security groups is null
(#515)
Posted by Andrew Phillips <no...@github.com>.
> @@ -530,6 +530,8 @@ public TemplateOptions nodeNames(Iterable<String> nodeNames) {
> * assigns the created nodes to these security groups
> */
> public TemplateOptions securityGroups(Iterable<String> securityGroups) {
> + for (String groupName : checkNotNull(securityGroups, "securityGroups"))
> + checkNotNull(groupName, "all security groups must be not null");
> this.securityGroups = ImmutableSet.copyOf(checkNotNull(securityGroups, "securityGroups"));
Don't need this `checkNotNull` any more.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/515/files#r17993591
Re: [jclouds] Throw sane error if one of security groups is null
(#515)
Posted by Andrew Phillips <no...@github.com>.
> @@ -538,6 +540,8 @@ public TemplateOptions securityGroups(Iterable<String> securityGroups) {
> * @see TemplateOptions#securityGroups(Iterable<String>)
> */
> public TemplateOptions securityGroups(String... securityGroups) {
> + for (String groupName : checkNotNull(securityGroups, "securityGroups"))
> + checkNotNull(groupName, "all security groups must be not null");
Do we need this here? Will the check in the other `securityGroups` method not catch this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/515/files#r17993575
Re: [jclouds] Throw sane error if one of security groups is null
(#515)
Posted by Everett Toews <no...@github.com>.
Thanks for the pull request but it's release week in jclouds and that means it's time to clean up the PR queue. This PR will be over 6 months old as of April 1. If you intend to continue work on it, please make a comment by April 2. Otherwise it will be closed on April 3.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/515#issuecomment-85715464