geomacy approved this pull request.
LGTM at a quick read. Still have a feeling it shouldn't throw exceptions if
`destroyNode` fails but will defer to you on this.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
geomacy commented on this pull request.
> @@ -95,6 +101,7 @@ public Boolean apply(String id) {
}
boolean serverDeleted =
novaApi.getServerApi(regionAndId.getRegion()).delete(regionAndId.getId());
+ checkState(nodeTerminatedPredicate.apply(id), "server was not destroyed
in
geomacy commented on this pull request.
> @@ -95,6 +101,7 @@ public Boolean apply(String id) {
}
boolean serverDeleted =
novaApi.getServerApi(regionAndId.getRegion()).delete(regionAndId.getId());
+ checkState(nodeTerminatedPredicate.apply(id), "server was not destroyed
in
[
https://issues.apache.org/jira/browse/JCLOUDS-1315?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Geoff Macartney updated JCLOUDS-1315:
-
Summary: Add InternetGatewayApi to AWSEC2Api (was: Add InternetGatewayApi)
>
[
https://issues.apache.org/jira/browse/JCLOUDS-1317?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Geoff Macartney resolved JCLOUDS-1317.
--
Resolution: Fixed
Resolved in
https://git-wip-us.apache.org/repos/asf?p=jclouds.git
Geoff Macartney created JCLOUDS-1317:
Summary: Add ModifySubnetAttribute to AWSSubnetApi
Key: JCLOUDS-1317
URL: https://issues.apache.org/jira/browse/JCLOUDS-1317
Project: jclouds
Issue
[
https://issues.apache.org/jira/browse/JCLOUDS-1316?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Geoff Macartney resolved JCLOUDS-1316.
--
Resolution: Fixed
Resolved in
https://git-wip-us.apache.org/repos/asf?p=jclouds.git
Geoff Macartney created JCLOUDS-1316:
Summary: Add a RouteTableApi to AWSEC2Api
Key: JCLOUDS-1316
URL: https://issues.apache.org/jira/browse/JCLOUDS-1316
Project: jclouds
Issue Type: New
[
https://issues.apache.org/jira/browse/JCLOUDS-1315?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Geoff Macartney resolved JCLOUDS-1315.
--
Resolution: Fixed
Resolved by
https://git-wip-us.apache.org/repos/asf?p=jclouds.git
Geoff Macartney created JCLOUDS-1315:
Summary: Add InternetGatewayApi
Key: JCLOUDS-1315
URL: https://issues.apache.org/jira/browse/JCLOUDS-1315
Project: jclouds
Issue Type: New Feature
hi @nacx, per our conversation on IRC I have reverted to using a custom
annotation, but put a
[comment](https://github.com/jclouds/jclouds/pull/1102/files#diff-d646d0465e56af59039c3489721bf61aR29)
on it explaining that it is meant only as a temporary measure to decouple the
new function in
Thanks @nacx, changes squashed.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1100#issuecomment-304282154
hi @nacx, @andreaturli, I hope this is good to go now, do you think it can be
merged?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1100#issuecomment-304269821
Note the new test util [enqueueXml(Response.Status, String,
String)](https://github.com/jclouds/jclouds/pull/1100/files#diff-8c2525316d73049be95880788b72ccd1R161)
in BaseAWSEC2ApiMockTest which may be of use elsewhere.
--
You are receiving this because you are subscribed to this thread.
Reply
hi @nacx, @andreaturli, I have pushed changes for the review comments made. For
convenience of review I have left these as separate commits. I can squash them
before merge.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
@geomacy pushed 13 commits.
8a84ac6 Remove autoBuild where not needed.
b6a3772 Remove unusable logger.
ac03910 Add 'origin' to Route and remove unused hidden accessors.
f02436d Remove another unused logger, and unused imports.
b002ed6 Have a separate RouteTableOptions and RouteOptions.
Thanks for the comments @andreaturli, @nacx, will address them as soon as I can
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1100#issuecomment-303331717
geomacy commented on this pull request.
> + @AutoValue.Builder
+ public abstract static class Builder {
+ public abstract Builder destinationCidrBlock(String
destinationCidrBlock);
+
+ public abstract Builder gatewayId(String gatewayId);
+
+ public abstract Builder
@nacx ok; I'll try to take a look at the tests to see if I can contribute
anything to a fix. I'd be keen to get this merged so I can start using it.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
Ah, so it's something to do with the tests themselves then, not just my
environment being wrong? I take it you get the error in `master` too?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
p.s. I have checked the same tests as above in `master` and they fail in the
same way for me, so it is something to do with my environment or AWS account,
not a feature of the PR itself.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view
hi @andreaturli, I've added a commit addressing the comments you made.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1100#issuecomment-302707715
@geomacy pushed 1 commit.
0248431 Add code review comments.
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1100/files/49c5c47adaf80efff8b722c3a3efd278ae2d4781..024843132066d56a2b9a3221edf03a964a275b5a
geomacy commented on this pull request.
> + @AutoValue.Builder
+ public abstract static class Builder {
+ public abstract Builder destinationCidrBlock(String
destinationCidrBlock);
+
+ public abstract Builder gatewayId(String gatewayId);
+
+ public abstract Builder
geomacy commented on this pull request.
> + return new AutoValue_RouteTableAssociation.Builder();
+ }
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+ public abstract Builder id(String id);
+ public abstract Builder routeTableId(String routeTableId);
+
@geomacy pushed 1 commit.
70860c7 Add comment explaining the @SinceApiVersion override.
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
hi @nacx
thanks for the explanation of the wiring.
I've tried to run live tests, but may not be set up for it - the tests have
`@SinceApiVersion` are
```
find . -name '*Api.java' | xargs grep -l SinceApiVersion | sed 's#.*/##' |
while read file ; do find . -name ${file%.java}LiveTest.java;
Updates made.
I was able to switch to using `@SinceApiVersion`, very pleased with that. The
[key
thing](https://github.com/jclouds/jclouds/pull/1102/commits/3bff59d28c5260e18504c6570ad5cb47117a3d48#diff-7d8365e45e83df7988b1084146439d71R158)
is that I needed to change the override from a
@geomacy pushed 3 commits.
3bff59d Use @SinceApiVersion rather than a custom annotation.
110b188 Use Optional rather than returning null.
74e4245 Apply the version check in FormSignerV2 as well as V4
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@nacx
I can add the annotation at class level.
I originally started using `@SinceApiVersion`, and having the annotation at
both method and class level, but found that various tests broke, because
various APIs are annotated `@SinceApiVersion`.I will take a look at that
again, but
This is a follow-up to
https://github.com/jclouds/jclouds/pull/1091#issuecomment-299202429.
> +1 to merging this but I have been trying this out and I think we will need
> to extend it for practical purposes; if you want to create a VPC and subnet
> and then depl>oy a machine on to it, you
@geomacy pushed 1 commit.
49c5c47 Add ReplaceRoute
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1100/files/b3d302d3f21e5853c517de7a1c5c2d6c35d0a6ea..49c5c47adaf80efff8b722c3a3efd278ae2d4781
geomacy commented on this pull request.
> + * (if needed):
+ *
+ *
+ * import static org.jclouds.ec2.options.RouteTableOptions.Builder.*
+ *
+ * EC2Api connection = // get connection
+ * RouteTable table =
connection.getRouteTableApi().get().createRouteTable(vpcId, dryRun());
+ *
+ *
+ *
geomacy commented on this pull request.
> @@ -40,17 +40,17 @@
/**
* The subnet is not yet available for use.
*/
- PENDING, UNRECOGNIZED;
Hm, hadn't meant to change VPC, just the classes I introduced - my thought was
to throw an exception rather than accept an
geomacy commented on this pull request.
> + @AutoValue.Builder
+ public abstract static class Builder {
+ public abstract Builder destinationCidrBlock(String
destinationCidrBlock);
+
+ public abstract Builder gatewayId(String gatewayId);
+
+ public abstract Builder
Implements the AWS RouteTable API.
Limitations:
- Does not contain ReplaceRoute.
- Does not contain support for VgwRoutePropagation, because I'm not too
familiar with this myself. I propose that this could be done as a later PR.
You can view, comment on, or merge this pull request online at:
@nacx thanks, changes squashed
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1097#issuecomment-300532323
hi @nacx I have made the changes you suggested, thanks very much for those.
They're done as individual commits for ease of review; I will squash this
before any merge, however.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on
Worth mentioning that I restricted the API to the general purpose gateway -
didn't do the egress only one. Could be added later.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
I fetched `master` and git status told me
modified:
providers/google-cloud-storage/src/test/resources/logback.xml
This just recommits the file with git configured as `core.autocrlf=input`
You can view, comment on, or merge this pull request online at:
This is a follow-up to https://github.com/jclouds/jclouds/pull/1091 and
particularly the comment at
https://github.com/jclouds/jclouds/pull/1091#issuecomment-299202429:
> I have been trying this out and I think we will need to extend it for
> practical purposes; if you want to create a VPC and
+1 to merging this but I have been trying this out and I think we will need to
extend it for practical purposes; if you want to create a VPC and subnet and
then deploy a machine on to it, you also need to jump through a few other hoops
apart from creating the subnet:
- modify the subnet
+1 looks good to me
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1091#issuecomment-298859600
geomacy commented on this pull request.
> +import static com.google.common.base.Preconditions.checkNotNull;
+
+import org.jclouds.ec2.options.internal.BaseEC2RequestOptions;
+
+/**
+ * Contains options supported in the Form API for the CreateSubnet
+ * operation.
+ * Usage The recommended way
geomacy commented on this pull request.
> @@ -220,10 +223,16 @@ void deleteSecurityGroup(String region, String group) {
checkNotNull(emptyToNull(group), "group must be defined");
String groupName = namingConvention.create().sharedNameForGroup(group);
- if
geomacy commented on this pull request.
> @@ -220,10 +223,16 @@ void deleteSecurityGroup(String region, String group) {
checkNotNull(emptyToNull(group), "group must be defined");
String groupName = namingConvention.create().sharedNameForGroup(group);
- if
geomacy commented on this pull request.
> +
+ /**
+* Creates a subnet in an existing VPC.
+*
+* @param region
+* @param vpcId The ID of the VPC.
+* @param cidrBlock The network range for the subnet, in CIDR notation. For
example, 10.0.0.0/24.
+* @param options
+
I can't really say if it's good to merge as such, as I haven't reviewed the
code in detail, but the updated java docs per our conversations do address the
point I made, so find by me.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it
geomacy commented on this pull request.
> @@ -75,13 +75,26 @@ public AzureTemplateOptions dataDisks(DataDisk...
> dataDisks) {
return dataDisks(ImmutableList.copyOf(checkNotNull(dataDisks,
"dataDisks")));
}
+ /**
+* Configure the NICs that will be attached to the created
geomacy commented on this pull request.
> @@ -75,13 +75,26 @@ public AzureTemplateOptions dataDisks(DataDisk...
> dataDisks) {
return dataDisks(ImmutableList.copyOf(checkNotNull(dataDisks,
"dataDisks")));
}
+ /**
+* Configure the NICs that will be attached to the created
geomacy commented on this pull request.
> @@ -75,13 +75,26 @@ public AzureTemplateOptions dataDisks(DataDisk...
> dataDisks) {
return dataDisks(ImmutableList.copyOf(checkNotNull(dataDisks,
"dataDisks")));
}
+ /**
+* Configure the NICs that will be attached to the created
> I agree that some OSs need to be prepared to enable the network interfaces on
> boot, but that can easily be achieved with bootstrap scripts and/or cloud
> init?
I guess so long as it is understood that this may be necessary, depending on
the image, then that's ok. It may be worth adding
@neykov this will interest you I'm sure
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/386#issuecomment-297717745
thanks @nacx !
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1064#issuecomment-281614735
[
https://issues.apache.org/jira/browse/JCLOUDS-1237?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15873367#comment-15873367
]
Geoff Macartney commented on JCLOUDS-1237:
--
I've PRed https://github.com/jclouds/jclouds/pull
Regarding https://issues.apache.org/jira/browse/JCLOUDS-1237
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1064
-- Commit Summary --
* Add compareTo() for IpPermission.
-- File Changes --
M
hi @andreaturli, changes squashed. Yes, it would be good to have this ported
to 2.0.x as well if that is possible.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
hi @andreaturli , @nacx , I have updates the PR with changes as suggested in
the comments above, improving the additional test and now deleting unused
classes.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
@geomacy pushed 2 commits.
23b60cd review comments and test fixes
33f1a74 Delete now-unused classes.
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
geomacy commented on this pull request.
> @@ -135,8 +136,9 @@ public NovaSecurityGroupExtension(NovaApi api,
}
Set groupNames = instance.getSecurityGroupNames();
- Set rawGroups =
-
geomacy commented on this pull request.
> @@ -153,7 +155,8 @@ public SecurityGroup getSecurityGroupById(String id) {
return null;
}
- SecurityGroupInRegion rawGroup = new
SecurityGroupInRegion(sgApi.get().get(groupId), region);
+ final
FluentIterable allGroups
=
Thanks for the comments @andreaturli and @nacx, - will add a couple of
responses above, and update the PR with corresponding changes.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
hi @nacx, thanks for the suggestions. A couple of thoughts/questions:
- Would the `Supplier` be a singleton similar to
[
https://issues.apache.org/jira/browse/JCLOUDS-1235?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15854616#comment-15854616
]
Geoff Macartney commented on JCLOUDS-1235:
--
I have raised a candidate fix for this in
https
Proposed fix for https://issues.apache.org/jira/browse/JCLOUDS-1235.
This change takes the approach of storing the information about the
overall list of groups within the `SecurityGroupInRegion` when it is
created, so that any subsequent conversion operation has access to all
the groups in the
Geoff Macartney created JCLOUDS-1235:
Summary: openstack-nova: list-security-groups request is slow,
with O(n^2) behaviour
Key: JCLOUDS-1235
URL: https://issues.apache.org/jira/browse/JCLOUDS-1235
Geoff Macartney created JCLOUDS-1234:
Summary: openstack-nova - Indeterminate/invalid group reference
created in ingress rule if duplicate groups in region
Key: JCLOUDS-1234
URL: https://issues.apache.org
Looks good; one thought - maybe it would be useful at this point to split the
feature file into several more modular files, e.g. at least to factor out the
jclouds labs features. Was going to suggest splitting apis and providers but
you will always need both, so not much point in that.
--
68 matches
Mail list logo
