Implementation for com.rultor.agents.ec2.Amazon interface - Issue #755 #760
Conversation
Remove todo annotation from Amazon
|
Thanks for your pull request, let me find someone who can review it |
|
@krzyk cuold you review this pull request |
| * Creates a new {@link RunInstancesRequest}. | ||
| * @return The {@link RunInstancesRequest} created | ||
| */ | ||
| private RunInstancesRequest createRequest() { |
|
@Marczeeee some comments above, and would be nice to have a test for this |
|
@krzyk I made the suggested changes in the code, and please see my comment about the waiting in execution. By the way it isn't so easy to test the functionality of this class as it performs direct request to Amazon to run a new EC2 instance. |
| final List<Instance> instances = client.runInstances( | ||
| this.create() | ||
| ).getReservation().getInstances(); | ||
| if ((instances != null) && !instances.isEmpty()) { |
There was a problem hiding this comment.
According to Amazon docs:
One or more instances.
So it shouldn't return null
There was a problem hiding this comment.
also, please also invert the if check ( not checks are confusing, that's why PMD complains) and remove Suppress from above
So the if would look like this:
if (instances.isEmpty()) {
throw new IllegalStateException("No Instance was available in the RunInstanceRequest reply!");
} else {
instance = instances.get(0);
}
|
@Marczeeee and more comment and we are ready to go |
|
@krzyk Done. |
|
@rultor merge pls |
| throw new IllegalStateException( | ||
| "No Instance was available in the RunInstanceRequest reply!" | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
| TimeUnit.SECONDS.sleep(Tv.FIFTEEN); | ||
| } catch (final InterruptedException ex) { | ||
| Thread.currentThread().interrupt(); | ||
| break; |
There was a problem hiding this comment.
why swallowing the exception? why can't we just throw it up, wrapped into IllegalStateException
|
@Marczeeee a few comments from me above. besides that, what about a test? how do you know all this works? |
|
@yegor256 The suggested changes are done. The problem with the test is that after calling 'runOnDemand()' it will connect to Amazon to run the instance. |
Isse #755