Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1499#issuecomment-221921405
@swill Sure can do, sorry for the delay, I haven't had much time lately but
that will be done soon.
---
If your project is set up for it, you can
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1280#issuecomment-217842674
as @pedro-martins stated, it seems to be fitting that this method is
extracted to a class to be documented/tested. The code looks good
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1357#issuecomment-217840055
Is there a test already to check if the ip ranges release method is called?
If there is none, I think it should be added
---
If your project is set
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1523#issuecomment-216192751
That seems fair... But I think you could make 3 unit tests instead of 1 so
each one would have only 1 assert and only one behavior each. Other than
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1503#discussion_r61723859
--- Diff: server/src/com/cloud/api/ApiServer.java ---
@@ -264,10 +266,11 @@ public void handleAsyncJobPublishEvent(String
subject, String
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1523#issuecomment-216070253
@cristofolini that would not be that simple, there is userDataApplied
variable which is extern to the loop. It would be better to turn lines
2496
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1516#discussion_r60852850
--- Diff: engine/schema/src/com/cloud/capacity/dao/CapacityDaoImpl.java ---
@@ -962,35 +962,59 @@ public boolean removeBy(Short
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1496#discussion_r60004052
--- Diff: server/src/com/cloud/ha/HighAvailabilityManagerImpl.java ---
@@ -264,6 +265,11 @@ public void scheduleRestartForVmsOnHost(final
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1491#discussion_r60003773
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -677,6 +679,17 @@ public boolean deleteAccount(AccountVO account, long
GitHub user alexandrelimassantana opened a pull request:
https://github.com/apache/cloudstack/pull/1499
Undetected bug correct and refactor of the class NfsSecondaryStorageResource
Im in process of rewriting the unit test for this class' mount method.
Before I can do
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1483#discussion_r59121072
--- Diff: server/src/com/cloud/network/element/VpcVirtualRouterElement.java
---
@@ -466,7 +466,7 @@ public boolean deletePrivateGateway
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1464#discussion_r58739597
--- Diff: tools/marvin/marvin/lib/vcenter.py ---
@@ -183,7 +192,157 @@ def get_clusters(self, dc, clus=None
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1464#discussion_r58633962
--- Diff: tools/marvin/marvin/lib/vcenter.py ---
@@ -183,7 +192,157 @@ def get_clusters(self, dc, clus=None
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1455#issuecomment-202874549
Does the _cleanup_resources(cls.api_client, cls._cleanup)_ cleans the db
connection as well? I see that it is only 1 test as of now
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1454#discussion_r57714466
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -2112,11 +2112,13 @@ private boolean doCancelMaintenance(final long
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1451#issuecomment-202161988
@cristofolini there is no _timeout_ variable in the scope you commented.
His change is valid because _VRScripts.DEFAULT_EXECUTEINVR_TIMEOUT_
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1402#issuecomment-201681267
@remibergsma can't you asking false instead of "false" ? If the function
accepts boolean values I think that it would be more
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1451#issuecomment-201610435
@insom with this changes there is still one place where the applyConfigToVR
is not provided with this timeout parameter (line 183, same class
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1445#discussion_r56808020
--- Diff: utils/src/test/java/com/cloud/utils/TestProfiler.java ---
@@ -19,84 +19,79 @@
package com.cloud.utils
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1410#issuecomment-199090954
Hello @ustcweizhou
I am curious to know if this segment is really necessary:
```Java
disk = new DiskTO(volumeTO, vol.getDeviceId
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1441#discussion_r56768712
--- Diff:
engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/StorageSystemSnapshotStrategy.java
---
@@ -440,41 +400,28
GitHub user alexandrelimassantana opened a pull request:
https://github.com/apache/cloudstack/pull/1445
Fixed Profiler's unit tests bugs.
Problem:
The TestProfiler class was using Java Thread methods to test the
Profiler's functionality. That was causing the tests to fail
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1212#discussion_r55956795
--- Diff: server/src/com/cloud/server/ManagementServerImpl.java ---
@@ -3567,7 +3567,17 @@ public boolean deleteSSHKeyPair(final
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/838#issuecomment-196137905
Hello @bvbharatk,
I think a simple change like this could have a test case to test it, so we
can avoid manual tests.
---
If your project
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1252#discussion_r55169762
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -4468,6 +4472,16 @@ private boolean checkIfHostIsDedicated(HostVO host
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1254#discussion_r55169349
--- Diff:
framework/quota/src/org/apache/cloudstack/quota/QuotaManagerImpl.java ---
@@ -358,10 +358,11 @@ public QuotaUsageVO
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/858#issuecomment-189993774
Hello @kansal
I think you just should test the possibility of setting the domainName as
null and see what happens.
With that tested
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1424#discussion_r54362124
--- Diff: server/src/com/cloud/api/ApiResponseHelper.java ---
@@ -1388,7 +1388,7 @@ public TemplateResponse
createTemplateUpdateResponse
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1424#discussion_r54362098
--- Diff: server/src/com/cloud/template/TemplateManagerImpl.java ---
@@ -1705,6 +1709,14 @@ public VMTemplateVO
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/876#discussion_r53570532
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -2378,6 +2378,23 @@ public boolean maintenanceFailed(final long
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1418#issuecomment-186875522
Hi @kansal, can you make a test with this change? Try issuing an request
with more than 2048 characters to see if everything still works fine
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1302#discussion_r53554647
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -2030,12 +2030,29 @@ int
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/874#discussion_r52846753
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -680,6 +680,13 @@ public Discoverer getMatchingDiscover(final
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/797#discussion_r52846855
--- Diff:
engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java ---
@@ -1776,19 +1773,26 @@ private void
Github user alexandrelimassantana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1410#discussion_r52846110
--- Diff:
engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
---
@@ -207,10 +211,14 @@ public
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/798#issuecomment-178042092
Hi @maneesha-p I see you did the extraction, but could you also write the
method javadoc along with it? As simple as it is, it can help people
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/892#issuecomment-177591098
@SudharmaJain could you turn that added piece of code into a method and
create some testCases? Manually testing works, but making some unit tests helps
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-176851830
@ustcweizhou there are some modifications you could do to make this code a
little more clean and readable.
**1)** You are duplicating code
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1246#issuecomment-174398371
Hello, there is also something bugging me with this piece of code which
appears on the changed files:
```java
final String gatewayCidr
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1302#issuecomment-174396396
@priyankparihar, since you are modifying this method, you could also
generate it's javadoc. It would be a nice upgrade from that single line comment
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/798#issuecomment-171739474
@maneesha-p you could extract the content of this if-condition into a
method to test the TaskInfo object, making the code a little more reusable
Github user alexandrelimassantana commented on the pull request:
https://github.com/apache/cloudstack/pull/1100#issuecomment-158764180
Thanks for that look-up @rafaelweingartner. Yeah that class was very old,
it's use in the hierarchy, as of now, was just for forwading an interface
GitHub user alexandrelimassantana opened a pull request:
https://github.com/apache/cloudstack/pull/1100
Removal of DefaultUserAuthenticator empty class.
The DefaultUserAuthenticator is an empty class, extending from the
AdapterBase and implementing the UserAuthenticator
GitHub user alexandrelimassantana opened a pull request:
https://github.com/apache/cloudstack/pull/855
Removal of class AgentBasedStandaloneConsoleProxyManager
The AgentBasedStandaloneConsoleProxyManager class is neither manually
instatiated anywhere in the code nor via Spring
GitHub user alexandrelimassantana opened a pull request:
https://github.com/apache/cloudstack/pull/852
Removal of DefaultUserAuthenticator empty class.
The DefaultUserAuthenticator is an empty class, extending from the
AdapterBase and implementing the UserAuthenticator interface
45 matches
Mail list logo
