Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220517071
?OK, I can create one and open a PR up against it reverting those variable
names.
From: Anshul Gangwar
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220516738
@mike-tutkowski No, I have not created ticket. You can create one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
I see that you just sent an e-mail to @dev about this - thanks!
From: mike-tutkowski
Sent: Thursday, May 19, 2016 10:46 PM
To: dev@cloudstack.apache.org
Subject: [GitHub] cloudstack pull request: Notify listeners when a host has
been add...
Github user
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220516362
Yes, I can send an e-mail out in a bit.
In the short term (at least for 4.9), I can just revert the changed names
in those Command classes.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220514841
@mike-tutkowski then we can raise this in dev list.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220514243
I wonder how many people are actually aware of that. :-)
Sent from my iPhone
On May 19, 2016, at 9:04 PM, Anshul Gangwar
mailto:notificati...
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220512436
@mike-tutkowski I believe class name ending in Command signifies that they
are meant for agent and should be treated like API params.
---
If your project is set
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220295817
I see.
Shouldn't the system be using annotations to make this less brittle.
At present, there's no obvious way to see that changing these vari
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220228493
@mike-tutkowski First one which I hit is in ModifyStoragePoolCommand.java.
But I am sure there will be more. Hyper-V uses json to communicate between
management s
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-220019059
Hi @anshul1886 Can you be more specific about the class that is given you
problems and the variables? Thanks!
---
If your project is set up for it, you can r
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-219949545
@koushik-das @swill @mike-tutkowski This has broken Hyper-V support. And
this is happening because of variable renaming in command.
---
If your project is set up
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/816
---
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 mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218553761
@swill OK, looks like are OK here.
---
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 pro
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218489553
I think we are definitely having issues with it not cleaning correctly. I
have seen a couple cases where we get the "unable to update index" when it
tries to do a git
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218480720
That is the rat test. it is usually right. I didn't see a new file without
license though. May one of the to with a license starting with an empty comment
line
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218465085
I have seen this happen periodically and it does not seem to be
consistently telling us the truth. This happens sometimes when only a single
line of code changes, so
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218463825
@swill Is this claiming there's a license problem?
I can rebase and push again.
[INFO] Rat check: Summary of files. Unapproved: 1 unknown: 1 g
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218456885
@mike-tutkowski unfortunately it failed again. :(
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If y
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218374540
@swill Done. Hopefully it will work for Jenkins this time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on Git
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218354565
Can you re-push to try to get Jenkins green. Thx...
---
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 swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218228412
The one issue showing in the CI run is an issue with my environment. I
think this one is ready to merge...
---
If your project is set up for it, you can reply to thi
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-218228119
### CI RESULTS
```
Tests Run: 85
Skipped: 0
Failed: 1
Errors: 0
Duration: 9h 15m 46s
```
**Summary of the pr
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62547345
--- Diff:
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java
---
@@ -91,18 +280,5 @@
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217947984
@syed Thanks for the code review!
@swill I believe we are good to go on this PR.
---
If your project is set up for it, you can reply to this email and hav
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62548252
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -1570,17 +1578,35 @@ private boolean checkCIDR(final HostPodVO pod,
final S
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62547209
--- Diff:
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java
---
@@ -65,22 +94,182 @
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62547042
--- Diff:
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java
---
@@ -18,40 +18,69 @@
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62546959
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -933,7 +937,7 @@ private PlugNicAnswer ex
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62546214
--- Diff: engine/components-api/src/com/cloud/agent/Listener.java ---
@@ -87,6 +93,18 @@
boolean processDisconnect(long agentId, Status sta
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62546243
--- Diff: engine/components-api/src/com/cloud/agent/Listener.java ---
@@ -87,6 +93,18 @@
boolean processDisconnect(long agentId, Status sta
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62545797
--- Diff:
engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/HypervisorHostListener.java
---
@@ -21,7 +21,13 @@
import com.clo
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217892225
This has the required code reviews. @mike-tutkowski have a look at the
comments @syed made. I will get this into my CI queue so we can get it moving
forward. Thanks
Github user syed commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217890036
@mike-tutkowski I've reviewed the code and it LGTM. I have a few minor
comments that should be fairly simple to adress.
---
If your project is set up for it, you can
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62513240
--- Diff: server/src/com/cloud/resource/ResourceManagerImpl.java ---
@@ -1570,17 +1578,35 @@ private boolean checkCIDR(final HostPodVO pod,
final String serv
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62511172
--- Diff:
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java
---
@@ -91,18 +280,5 @@ public bo
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62509977
--- Diff:
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java
---
@@ -65,22 +94,182 @@ public b
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62509667
--- Diff:
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java
---
@@ -18,40 +18,69 @@
*/
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62508121
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -933,7 +937,7 @@ private PlugNicAnswer execute(Plug
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62505386
--- Diff: engine/components-api/src/com/cloud/agent/Listener.java ---
@@ -87,6 +93,18 @@
boolean processDisconnect(long agentId, Status state);
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62505426
--- Diff: engine/components-api/src/com/cloud/agent/Listener.java ---
@@ -87,6 +93,18 @@
boolean processDisconnect(long agentId, Status state);
Github user syed commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r62505096
--- Diff:
engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/HypervisorHostListener.java
---
@@ -21,7 +21,13 @@
import com.cloud.excepti
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217437287
Thanks for your time on the review, @miguelaferreira!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub a
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217370720
Hi @mike-tutkowski,
I trust you either addressed all my previous remarks, or have a good reason
not to.
I've skimmed (very quickly through the di
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217226582
@miguelaferreira I went ahead and added four integration tests to
/test/integration/plugins/solidfire.
Please let me know if you're OK with the PR. I
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217091053
I think `/test/integration/plugins/solidfire` is a good place to keep the
integration tests. I wouldn't disable or skip them because, IMH, it's not hard
to s
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-217018726
hi @miguelaferreira Yeah, after working on this PR last September, I got
side tracked with other, higher-priority items and haven't gotten back to it
until re
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-216987677
Hi @mike-tutkowski
It has indeed been a long time :)
None the less, it's great that you took the time time write that
integration test. I think i
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-216970070
@miguelaferreira I know it's been a while since you and I were talking
about this. I attached a Marvin test for this, but I don't have a way to run it
yet wit
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-216337441
Here are the results of my automated regression test:
test_add_remove_host_with_solidfire_plugin
(TestAddRemoveHosts.TestAddRemoveHosts) ... === TestN
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-216327328
Here is an integration test for this:
[TestAddRemoveHosts.py.txt](https://github.com/apache/cloudstack/files/245702/TestAddRemoveHosts.py.txt)
---
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-216189633
@mike-tutkowski please rebase against latest master and push -f, update on
status of your PR
---
If your project is set up for it, you can reply to this email and ha
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-144619954
So, I've been paging through the diff files to see what might be a good
candidate for unit testing.
Unfortunately, I don't really see much.
W
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-142081363
I understand.
I'm working on a task from last week and once I complete it I can take a
stab at some until tests for this PR.
---
If your project is
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-141960991
Hi @mike-tutkowski
Unfortunately, I don't think I will have the time to help you with unit
tests for this PR.
I will again need to direct my sp
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-140189221
Just an FYI that I went ahead and updated the code to remove the
commented-out lines.
---
If your project is set up for it, you can reply to this email and h
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-140097887
Sure, I'd be happy to get your input on how you see us unit testing this
code. In the past, I've mainly restricted my CloudStack testing (automated and
manual
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-140095795
Hi @mike-tutkowski
Thanks for being open about removing the commented-out code. IMHO (and not
just mine btw) commented-out code poses a serious risk
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-140080679
Hi Miguel,
I can remove those commented-out lines. No problem. Some companies like
such lines left in, but others - as you point out - like them remov
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39394113
--- Diff:
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
---
@@ -124,6 +132,22 @@
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-140077337
Hi again Mike,
Thanks for addressing my questions.
Regarding the commented-out code, I see from your answers that the reason
for leaving com
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-140071482
Hi Miguel,
Fortunately this PR is such that there is essentially no existing test code
that needed to be modified. At a high level, this code simply g
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390959
--- Diff: server/src/com/cloud/storage/upload/UploadListener.java ---
@@ -113,7 +113,7 @@ protected void runInContext() {
private Dat
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390966
--- Diff: server/src/com/cloud/storage/upload/UploadListener.java ---
@@ -270,7 +274,7 @@ public AgentControlAnswer processControlCommand(long
agen
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390917
--- Diff: server/src/com/cloud/storage/LocalStoragePoolListener.java ---
@@ -39,7 +39,7 @@
import com.cloud.utils.db.DB;
public clas
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390851
--- Diff: server/src/com/cloud/storage/LocalStoragePoolListener.java ---
@@ -18,7 +18,7 @@
import javax.inject.Inject;
-import
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390730
--- Diff: server/src/com/cloud/network/security/SecurityGroupListener.java
---
@@ -106,7 +106,7 @@ public boolean processAnswers(long agentId, long
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390723
--- Diff: server/src/com/cloud/network/security/SecurityGroupListener.java
---
@@ -81,7 +81,7 @@ public boolean isRecurring() {
@Override
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390674
--- Diff:
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1981,7 +1981,7 @@ private void createDefaultEgressFir
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390690
--- Diff:
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1971,7 +1971,7 @@ protected void finalizeNetworkRules
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390601
--- Diff:
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1971,7 +1971,7 @@ protected void finalizeNetworkRules
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390513
--- Diff:
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1589,8 +1589,8 @@ protected StringBuilder createGuest
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390303
--- Diff:
plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/discoverer/XcpServerDiscoverer.java
---
@@ -115,7 +115,7 @@
pr
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390243
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -4420,11 +4450,13 @@ private void fillHos
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39390123
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -3327,7 +3330,7 @@ private Answer execute
Github user wido commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39370166
--- Diff:
plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
---
@@ -124,6 +132,22 @@
private
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-139986534
Hi @mike-tutkowski
I've skimmed through the code in this PR and I saw quite a lot of nice
modifications. Things like proper formatting, stricter obj
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366965
--- Diff: server/src/com/cloud/storage/LocalStoragePoolListener.java ---
@@ -39,7 +39,7 @@
import com.cloud.utils.db.DB;
public cla
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366998
--- Diff: server/src/com/cloud/storage/upload/UploadListener.java ---
@@ -113,7 +113,7 @@ protected void runInContext() {
private Da
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39367006
--- Diff: server/src/com/cloud/storage/upload/UploadListener.java ---
@@ -270,7 +274,7 @@ public AgentControlAnswer processControlCommand(long
age
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366953
--- Diff: server/src/com/cloud/storage/LocalStoragePoolListener.java ---
@@ -18,7 +18,7 @@
import javax.inject.Inject;
-import
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366926
--- Diff: server/src/com/cloud/network/security/SecurityGroupListener.java
---
@@ -81,7 +81,7 @@ public boolean isRecurring() {
@Override
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366920
--- Diff:
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1981,7 +1981,7 @@ private void createDefaultEgressFi
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366935
--- Diff: server/src/com/cloud/network/security/SecurityGroupListener.java
---
@@ -106,7 +106,7 @@ public boolean processAnswers(long agentId, lon
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366912
--- Diff:
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1589,8 +1589,8 @@ protected StringBuilder createGues
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366916
--- Diff:
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java ---
@@ -1971,7 +1971,7 @@ protected void finalizeNetworkRule
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366830
--- Diff:
plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/discoverer/XcpServerDiscoverer.java
---
@@ -115,7 +115,7 @@
p
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366816
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -4420,11 +4450,13 @@ private void fillHo
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/816#discussion_r39366792
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -3327,7 +3330,7 @@ private Answer execut
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/816#issuecomment-139978868
Just an FYI that I have tested this with the two SolidFire storage plug-ins
(where applicable) making use of the following hypervisor types and versions:
GitHub user mike-tutkowski opened a pull request:
https://github.com/apache/cloudstack/pull/816
Notify listeners when a host has been added to a cluster, is about toâ¦
⦠be removed from a cluster, or has been removed from a cluster
This PR addresses the following JIRA tic
90 matches
Mail list logo
