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 is
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 \
component/test_
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 this
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 camelCase
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 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 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 your
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 we
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 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 G
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 f
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 no
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 ch
13 matches
Mail list logo
