Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1108#issuecomment-162231828
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1108
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1108#issuecomment-161927037
There is a lot of LGTM here (including mine) but did anybody do any
relevant tests with this code?
---
If your project is set up for it, you can reply to
Github user jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1108#issuecomment-161877685
@rafaelweingartner I agree. The use of "_" has no place in modern Java
code. Constants should be all caps with _ separators. Everything else should
be
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1108#issuecomment-161353281
We've used _var for global or class variables throughout the codebase,
should we keep it?
LGTM (only code review) otherwise
---
If your project is set up for
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1108#issuecomment-161388418
@bhaisaab, I assume the use of _ was introduced probably by someone that
comes from a C/C++ background. IMHO that does not make much sense when coding
in
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1108#issuecomment-161117310
reviewed again after update, still lgtm
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1108#discussion_r46260293
--- Diff: api/src/com/cloud/deploy/DeploymentPlanner.java ---
@@ -1,323 +1,306 @@
-// Licensed to the Apache Software Foundation (ASF) under one
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1108#issuecomment-160973087
@DaanHoogland problem solved with that class.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1108#issuecomment-160599664
Code seems good, waiting for @borisroman to come up with test results
---
If your project is set up for it, you can reply to this email and have your
reply appear on
GitHub user pedro-martins opened a pull request:
https://github.com/apache/cloudstack/pull/1108
Removed the PlannerBase class because it is does not bring contribution to
ACS' code.
Removed the PlannerBase class because it is does not bring contribution to
ACS' code.
We
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1108#issuecomment-158952872
less is more. Are there no other types of Planner{}s then
DeploymentPlanner{}s? and in addition Interfaces are a betterway to signify
types if/when there is
Github user borisroman commented on the pull request:
https://github.com/apache/cloudstack/pull/1108#issuecomment-158981153
I agree with @DaanHoogland, less is more! Thanks for your contribution.
I'll run integration tests on it to show nothing breaks.
---
If your project is set up
13 matches
Mail list logo
