JCLOUDS-208: Changes for the Abiquo 2.4 procedure to limit enterprises to datacenters #17

Closed
wants to merge 3 commits into
from

Projects

None yet

4 participants

@carlosgarciaibanez
Contributor

The datacenter link has been added to the Limits object construction. This link is used in the 2.4 version of the Abiquo API to establish a relation between an enterprise and the datacenter it is restricted to.

Carlos Garcia JCLOUDS-208: The datacenter link has been added to the Limits object …
…construction. This link is used in the 2.4 version of the Abiquo API to establish a relation between an enterprise and the datacenter it is restricted to
320a74d
@buildhive

jclouds » jclouds-labs #225 SUCCESS
This pull request looks good
(what's this?)

@nacx nacx and 1 other commented on an outdated diff Jul 30, 2013
...java/org/jclouds/abiquo/domain/enterprise/Limits.java
super();
this.context = context;
+ this.datacenter = datacenter;
@nacx
nacx Jul 30, 2013 Member

Since this field is mandatory, could you add a checkNotNull check for it?

@demobox
demobox Jul 31, 2013 Member

Same goes for context, incidentally, although that should probably be a separate commit. And the context and datacenter fields could also be final, it seems.

What is the call to super() for?

@nacx
Member
nacx commented Jul 30, 2013

Just a quick note on backwards compatibility (because the issue mentions Abiquo 2.4), this change should not affect previous Abiquo versions, since older versions of the API will just ignore the new link.

This PR looks good to me, once the null check has been added to the builder. Thanks @carlosgarciaibanez!

And a tip: Next time (there is no need to change that now), it is better to create a branch in your fork for the changes, and don't work directly on master. You can send the PR from your topic branch.

@demobox demobox commented on the diff Jul 31, 2013
...java/org/jclouds/abiquo/domain/enterprise/Limits.java
@@ -96,7 +110,8 @@ public Limits build() {
}
public static Builder fromEnterprise(final Limits in) {
- return Limits.builder(in.context).ramLimits(in.getRamSoftLimitInMb(), in.getRamHardLimitInMb())
+ return Limits.builder(in.context, in.getDatacenter())
+ .ramLimits(in.getRamSoftLimitInMb(), in.getRamHardLimitInMb())
@demobox
demobox Jul 31, 2013 Member

If this is purely a formatting change, align with the following lines?

@demobox demobox commented on the diff Jul 31, 2013
...java/org/jclouds/abiquo/domain/enterprise/Limits.java
@@ -57,8 +64,8 @@ public void update() {
// Builder
- public static Builder builder(final ApiContext<AbiquoApi> context) {
- return new Builder(context);
+ public static Builder builder(final ApiContext<AbiquoApi> context, Datacenter datacenter) {
@demobox
demobox Jul 31, 2013 Member

I assume there is no way to provide a backwards-compatible version of this call? Then again, this is master, so probably no need...

@nacx
nacx Aug 1, 2013 Member

There is no backwards compatible version, but I don't think it should be a problem. This builder will rarely be used directly, since the functionality is usually invoked from the Enterprise.allowDatacenter method.

Carlos Garcia added some commits Jul 31, 2013
@carlosgarciaibanez
Contributor

The suggested changes have been included. Thank you all for the tips and the support!

@buildhive

jclouds » jclouds-labs #230 SUCCESS
This pull request looks good
(what's this?)

@nacx
Member
nacx commented Aug 1, 2013

Squashed the commits and merged to master and 1.6.x.
Thanks @carlosgarciaibanez!

@nacx nacx closed this Aug 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment