Github user ProjectMoon closed the pull request at:
https://github.com/apache/cloudstack/pull/1491
---
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
GitHub user ProjectMoon reopened a pull request:
https://github.com/apache/cloudstack/pull/1491
Fixes regarding VOLUME_DELETE events resulting from account deletion.
New version of #1373, but updated for the 4.7 branch with another fix that
allows it to properly find expunged root
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1491#issuecomment-221916927
@ProjectMoon and again. :( sorry...
---
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
Github user ProjectMoon closed the pull request at:
https://github.com/apache/cloudstack/pull/1491
---
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
GitHub user ProjectMoon reopened a pull request:
https://github.com/apache/cloudstack/pull/1491
Fixes regarding VOLUME_DELETE events resulting from account deletion.
New version of #1373, but updated for the 4.7 branch with another fix that
allows it to properly find expunged root
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1491#issuecomment-221873700
@ProjectMoon try to close and reopen this one to try to get it green.
thx...
---
If your project is set up for it, you can reply to this email and have your
reply
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1491#issuecomment-221846283
Rebased against latest 4.7.
---
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
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1491#issuecomment-221763412
This one needs review as well...
---
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
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1491#issuecomment-218858373
yep, sounds good...
---
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
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1491#issuecomment-218848337
so @swill, I may find time to review later. In the meanwhile let's use
tag:needsreview
makes sense?
---
If your project is set up for it,
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1491#issuecomment-218838932
Can I get some code review on this PR? 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/1491#issuecomment-218838753
### CI RESULTS
```
Tests Run: 85
Skipped: 0
Failed: 2
Errors: 0
Duration: 4h 19m 11s
```
**Summary of the
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 cristofolini commented on the pull request:
https://github.com/apache/cloudstack/pull/1491#issuecomment-211140525
@ProjectMoon May I suggest extracting lines 778-784 in `AccountManagerImpl`
to a separate method? That would allow you to write your comment in a proper
Github user nnesic commented on the pull request:
https://github.com/apache/cloudstack/pull/1491#issuecomment-209922376
@koushik-das We are mostly working with 4.7 version, where other volume
usage events get emitted correctly. We haven't investigated the behavior on
master yet.
Github user nnesic commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1491#discussion_r59707938
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -761,6 +774,17 @@ protected boolean cleanupAccount(AccountVO account,
long
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1491#issuecomment-209830019
@ProjectMoon On latest master during VM deployment no VOLUME.CREATE is
getting generated for ROOT volume. Also during destroy there is no
VOLUME.DELETE event.
Github user pdube commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1491#discussion_r59644462
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -761,6 +774,17 @@ protected boolean cleanupAccount(AccountVO account,
long
Github user pdube commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1491#discussion_r59644306
--- Diff: server/src/com/cloud/user/AccountManagerImpl.java ---
@@ -761,6 +774,17 @@ protected boolean cleanupAccount(AccountVO account,
long
GitHub user ProjectMoon opened a pull request:
https://github.com/apache/cloudstack/pull/1491
Fixes regarding VOLUME_DELETE events resulting from account deletion.
New version of #1373, but updated for the 4.7 branch with another fix that
allows it to properly find expunged root
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1373#issuecomment-209361378
We have found that this fix actually does not work on 4.7+. We are going to
submit a new pull request based on 4.7 with a working fix.
---
If your project is
Github user ProjectMoon closed the pull request at:
https://github.com/apache/cloudstack/pull/1373
---
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
Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1373#issuecomment-198312562
### ACS CI BVT Run
**Sumarry:**
Build Number 110
Hypervisor xenserver
NetworkType Advanced
Passed=104
Failed=15
Skipped=4
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1373#issuecomment-182301032
Even for forward-merged bug fixes?
---
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 wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1373#issuecomment-182267349
@ProjectMoon Currently waiting for a new Release Manager for version 4.9 to
pick this up
---
If your project is set up for it, you can reply to this email and have
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1373#issuecomment-179181011
Who gets to be the authority on merging this?
---
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 DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1373#issuecomment-177461984
[1373.network.results.txt](https://github.com/apache/cloudstack/files/111219/1373.network.results.txt)
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1373#issuecomment-176670774
I added the suggested changes. Also, now that #1382 has been merged to 4.6,
the checks should succeed.
---
If your project is set up for it, you can reply to
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1373#issuecomment-176700390
Congratulations, the code is much better now.
I can give LGTM now without doubt
---
If your project is set up for it, you can reply to this email
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1373#issuecomment-176270996
@ProjectMoon that is great,
Your code is ok,
I would just suggest you extracting the code from "AccountManagerImpl"
lines 769-778 to a method.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1373#discussion_r51146580
--- Diff: server/test/com/cloud/user/AccountManagerImplTest.java ---
@@ -205,6 +223,11 @@
@Mock
private UserAuthenticator
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1373#issuecomment-176263770
@rafaelweingartner I have removed the MockUsageEventDao and replaced it
with a normal Mockito mock of the UsageEventDao interface.
---
If your project is set
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1373#discussion_r51146522
--- Diff: server/test/com/cloud/user/AccountManagerImplTest.java ---
@@ -192,6 +205,11 @@
@Mock
VMSnapshotDao _vmSnapshotDao;
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1373#issuecomment-176205293
Hi @ProjectMoon ,
Out of curiosity, why did you create the
"server/test/com/cloud/user/MockUsageEventDao.java" class?
---
If your project is set up
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1373#issuecomment-176208728
I'm not the original author of this work, but I think it was just because a
class for testing was needed. Looking at it now I'm not really sure why it
can't
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1373#issuecomment-176200921
Added the license header to the MockUsageEventDao class to fix rat report
error.
---
If your project is set up for it, you can reply to this email and have
GitHub user ProjectMoon opened a pull request:
https://github.com/apache/cloudstack/pull/1372
Fixes regarding VOLUME_DELETE events resulting from account deletion.
New version of #924, but on the right branch with the commits squashed.
**Original pull request:**
Fixes
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1372#issuecomment-175712011
Yes.. wrong branch again. GitHub's cross-fork interface tends to be a bit
confusing for me due to how it loads when you change repos. Attempt #3 coming
up.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1372#issuecomment-175716111
@ProjectMoon :) Github need to fix this, really annoying
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1372#issuecomment-175706330
Which file is missing the license? Still don't see that in the Jenkins
build.
---
If your project is set up for it, you can reply to this email and have your
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1372#issuecomment-175710489
@ProjectMoon do you want to merge this only on master or since 4.6/4.7?
Check the target branch of the PR.
---
If your project is set up for it, you can reply to
Github user ProjectMoon closed the pull request at:
https://github.com/apache/cloudstack/pull/1372
---
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
GitHub user ProjectMoon opened a pull request:
https://github.com/apache/cloudstack/pull/1373
Fixes regarding VOLUME_DELETE events resulting from account deletion.
New version of #924, but on the right branch with the commits squashed.
Original pull request:
Fixes
43 matches
Mail list logo
