Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1487#issuecomment-208924288
Ok, perfect. I will merge this now. Thx for confirming.. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1459#issuecomment-208987688
I have tried a few times, but I also was having problems with my CI when
trying to test this. I was getting errors, but I was trying to determine if it
was my env
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1445#issuecomment-209020807
@pdube we need to run CI against it first, but otherwise, yes it is ready...
---
If your project is set up for it, you can reply to this email and have your
reply
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1489#issuecomment-209021645
This is a feature that we definitely need. Can I please get some code
review on this PR?
---
If your project is set up for it, you can reply to this email and have
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1445#issuecomment-209025189
@pdube sorry, my brain is into something else right now and I didn't review
the scope of the code. Yes, your tests are valid tests for this change. So
yes, I
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1445#issuecomment-209028959
I appreciate it. :+1:
---
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 swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1263#issuecomment-209095939
This one looks like it is ready to go now. I will add it to my next 'ready
to merge' batch. Thanks again for all the hard work @GabrielBrascher. :+1:
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-209224739
So do we have 2 LGTM code reviews for this then? @jburwell do you give
this a +1?
---
If your project is set up for it, you can reply to this email and have your
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-209461264
Ok, thanks. I will add to merge list.
---
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 swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1412#issuecomment-209530633
What do we do about this since it is against the 4.6 branch? Do you want
to close this and open it against the 4.7 branch?
---
If your project is set up for it
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1406#issuecomment-209533528
Yes exactly, but the oldest supported branch right now is 4.7.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1406#issuecomment-209531960
another one against 4.6. how should we be handling this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1403#issuecomment-210009483
This does not compile:
```
[ERROR] COMPILATION ERROR :
[INFO] -
[ERROR]
/data/git/cs1
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1403#issuecomment-210012980
Looking closer at this code. It look like `primaryStoreDriver` could
easily be `null` in this case, so that return statement could potentially cause
a null pointer
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1403#issuecomment-210085510
i will blow away my git repo and start from scratch and we will go from
there. :) thx...
---
If your project is set up for it, you can reply to this email and
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59792529
--- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java ---
@@ -19,146 +19,198 @@
package com.cloud.utils.testcase
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59799743
--- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java ---
@@ -19,146 +19,198 @@
package com.cloud.utils.testcase
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1452#issuecomment-210646612
It seems to be in pretty good order. I would like some code reviews and
since there are changes to logic outside of the tests, I would like a full CI
run against it
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1452#issuecomment-211370314
I think this is ready pending 1 more LGTM code review...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1489#issuecomment-211374296
Nice work on the quality code reviews guys. @DaanHoogland since there have
been a bunch of changes, would you mind running your CI against this one? I am
sorting
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1423#issuecomment-211375246
Introducing an external dependency is probably going to cause some
problems. @bhaisaab can you review this and maybe give some feedback on the
best way to handle
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1475#issuecomment-211417769
@remibergsma how should we handle this PR now that we have found references
to these files?
---
If your project is set up for it, you can reply to this email and
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-211419101
I need one more LGTM code review of this one, otherwise it is looking to be
in good order...
---
If your project is set up for it, you can reply to this email and
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-211436333
@rafaelweingartner I understand you feel like you have a conflict of
interests, but at the same time, you are also very familiar with the code so
are in a good
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-211441451
Ok perfect, thanks @rafaelweingartner. I will get this merged...
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1423#issuecomment-211502237
@bhaisaab Ok thanks. Are there other things we want to get into the
systemvm template if we do publish a new one? Probably not the best place for
this discussion
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1423#issuecomment-211537431
Ok, thanks @bhaisaab. So I need to get on the systemvm template stuff asap
to try to get those PRs in a good place so we have a little breathing room
before the RC
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/872#issuecomment-211539172
I would like to get on top of the PRs that have systemvm changes, because
it looks like we will have to push new systemvms for the 4.9 release. We still
have a bit
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1371#issuecomment-211541350
Can you rebase this PR based on the current master so we can resolve the
conflicts so we can get CI running against this again. I want to do a push on
the PRs that
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1500#issuecomment-211551366
would you mind squashing your commits for this PR?
---
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/1365#issuecomment-211554643
I think this one is pretty close. I want to run CI against it since there
have been changes since the last CI run. I also need some LGTM code reviews.
I don'
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1402#issuecomment-211562114
@remibergsma can you review the functionality you expect. I think the
logic is wrong, and if it is not, then it is VERY misleading.
The code in question
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1454#issuecomment-211563367
@abhinandanprateek, I believe you are away this week, but can your address
@jburwell's comments when you have a chance. Thanks.
---
If your project is set u
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1473#issuecomment-211563985
@remibergsma apparently Jenkins doesn't like us much now days. Can you do
a force push again for me. Also, I am looking for another LGTM on this PR.
T
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1365#issuecomment-211565374
@rafaelweingartner I am not sure. @bhaisaab would you mind clarifying for
us. I am trying to stay on top of this stuff, but I am very much playing
catchup
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1420#issuecomment-211706237
## CI RESULTS
### 84/85 TESTS PASSED
The test that failed is a test that commonly fails in my environment and
has been
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1420#issuecomment-211706355
## CI RESULTS
### 84/85 TESTS PASSED
The test that failed is a test that commonly fails in my environment and
has been
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1420#issuecomment-211706541
With these results, I think this PR is ready. I will add it to my merge
list.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1420#issuecomment-211706714
Do I need to do anything special with this merge because it includes
systemvm changes? If I need to do anything special, please help me understand
the details
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1397#issuecomment-211725428
@bhaisaab I am still getting compile errors. I will reconfigure my CI
environment to use an older version of Java to see if that fixes the problem
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1489#issuecomment-211911369
This failed to compile. Can you have a look?
```
[INFO] --- exec-maven-plugin:1.2.1:exec (compile) @ cloud-apidoc ---
log4j:WARN No appenders could
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1420#issuecomment-211915697
@mlsorensen did you do a code review of this or are you just excited about
the change. :)
Ideally I would get another LGTM before merging. I am pretty
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1402#issuecomment-211916588
Thanks. Looks good now...
---
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
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1376#issuecomment-211917846
Thanks koushik. I tend to agree with you on this one. I will add it to my
merge list. Thanks...
---
If your project is set up for it, you can reply to this email
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1375#issuecomment-211918463
This one looks like it will have merge conflicts with #1376.
@milamberspace should I first merge #1376 and then have you rebase this PR and
fix the conflicts before
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1394#issuecomment-211926700
@koushik-das I am not seeing the commit in master. Did you merge into 4.7
and then forward merge into 4.8 and then forward merge into master???
---
If your project
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1420#issuecomment-211930508
@bhaisaab I will let you address @rafaelweingartner's comment. If you
choose to make a change, it will not impact the 'readiness' of this PR. I am
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1397#issuecomment-211932110
@bhaisaab thank you sir. sorry this is proving to be a bit complicated. i
appreciate the support.
---
If your project is set up for it, you can reply to this
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1459#issuecomment-211936017
## CI RESULTS
### 84/85 TESTS PASSED
The test that failed is a test that commonly fails in my environment and
has been
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1394#issuecomment-211947137
Every commit that goes into 4.7 gets fwd-merged into 4.8 and then gets
fwd-merged into master. We have tools in `cloudstack/tools/git/` which make
this process MUCH
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1394#issuecomment-211958527
@koushik-das oh, i understand your question better now. i would like to do
it on every commit. It makes the commit history clearer and it mitigates
conflict issues
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211962282
@nvazquez I have added this to my CI queue. I have a pretty serious
backlog, so it may take me a few days to get to it though. In the mean time,
lets see if we can
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1459#issuecomment-211991515
I have one LGTM. I need one more and this PR is ready to merge. Thanks
guys...
---
If your project is set up for it, you can reply to this email and have your
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1420#issuecomment-212040090
@bhaisaab I think you are being too hard on @rafaelweingartner. He simply
asked a question because the format was presented differently in two different
files in
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1489#issuecomment-212064683
I was confused by this as well. I will kick off another CI run against
this PR and see if I have the problem again.
---
If your project is set up for it, you can
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1489#issuecomment-212071544
@bhaisaab my environment builds it twice. Once with packaging and once
without. If I have the problem again, I will send you the command that was run
that had the
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1420#issuecomment-212074504
@bhaisaab thank you, I appreciate the effort. ð
I am actively trying to get more people involved in code review, so we will
have to lead by example for
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1489#issuecomment-212106389
The tests are now being run, so this time everything built alright. I will
have results for you later tonight or in the morning. :)
---
If your project is set up
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1489#issuecomment-212129730
@borisstoyanov awesome, thanks. Are you able to push the test results to
the PR? I have built a tool (https://github.com/cloudops/upr) to hopefully
simplify the
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1376#issuecomment-212530190
Jenkins failed with a timeout (not related to your code). Can you squash
your commits and do a force push again? Sorry for the runaround..
---
If your project is
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1157#issuecomment-212531389
I think we are just missing 1 LGTM for this one.
---
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/1500#issuecomment-212533078
We need some LGTM code reviews of this one. I would also like to run CI
against it because it changes code that is common to other functionality to
make sure
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/883#issuecomment-212537408
I will admit, I don't really understand everything in play here. If I read
this quickly, it seems like this PR may not be needed because #861 essentially
change
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1459#issuecomment-212541636
I think this one is ready now. I will add it to my merge queue.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1376#issuecomment-212585886
@DaanHoogland do you know why jenkins is failing here?
---
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 swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1505#issuecomment-212588764
Since this is a UI change, is it possible to supply screenshots to show
this behaves as expected? For UI changes we tend to use visual proof since we
can't
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1323#issuecomment-212902454
I think this one is ready to merge. I will add it to my merge queue...
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1200#issuecomment-212903509
Thanks, I will merge...
---
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 swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1464#issuecomment-212904531
I am fine with not running CI on this one. It would be nice if someone
could post a screenshot or something to show that the code behaves as expected.
We
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1308#issuecomment-212905418
I will merge this today. Thanks...
---
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/1458#issuecomment-212906931
I think this one is ready. I want Jenkins to be green before I merge
though. I will review what is going on with Jenkins today. I will this to my
merge list
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1326#issuecomment-212910562
Thanks, I will merge this today.
---
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/1501#issuecomment-212908719
I am pretty confident with this one and we have the LGTM code reviews
required, so I will add this to my merge queue to be merged today. Thanks...
---
If your
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1304#issuecomment-212934000
@sanju1010 can you review @pavanb018's comment.
We need 1 more LGTM code review, otherwise things are looking to be in
pretty good shape here.
---
If
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1505#issuecomment-212956256
Thank you sir. This is looking pretty good now. I think this LGTM, so I
will add this to the PRs to be merged today.
---
If your project is set up for it, you can
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/826#issuecomment-212963691
I am pretty comfortable with this change. We have had 2 manual tests to
validate the functionality and two LGTM votes. I am not sure this change
warrants a full run
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1498#issuecomment-212964858
I need one more LGTM and if you can post some proof this is working as
expected, I will feel better about merging it. Thanks... :)
---
If your project is set up
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1126#issuecomment-212976859
We need to be doing this to more API calls, so I am happy we are starting
to look at this.
@DaanHoogland I don't think JSON adds much value here. I
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1054#issuecomment-212981219
I need one more LGTM for this one and I would like to run CI to make sure
nothing breaks. Otherwise I think this is in a good place.
---
If your project is set up
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/846#issuecomment-212983883
I need one more LGTM. I will add this to my CI queue. Thx...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1498#issuecomment-212985885
That is what I was looking for. Thank you sir... :)
---
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 swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1271#issuecomment-212987286
@footplus Is that a LGTM? I need one more LGTM code review.
@anshul1886, can you please force push to the PR again to kick of jenkins
so we can get that to
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/846#issuecomment-213034173
Thank you @GabrielBrascher. @kishankavala would you mind fixing that typo?
I will try to get this run though CI soon...
---
If your project is set up for it, you
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1376#issuecomment-213100441
That is fine. @milamberspace would you mind just doing a force push again
to kick off jenkins. I have seen other jenkins runs passing so I think jenkins
is just
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1376#discussion_r60653156
--- Diff: tools/transifex/.tx/config ---
@@ -89,3 +89,41 @@ trans.pt_BR = work-dir/messages_pt_BR.properties
trans.ru_RU = work-dir
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/713#issuecomment-213407542
I need one more LGTM code review on this one. I will try to test this in
my lab today. Thanks...
---
If your project is set up for it, you can reply to this email
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1417#issuecomment-213408951
I need one more LGTM code review on this one. I will try to test this in
my lab today. Can you do a force push of this PR again to kick off jenkins so
we can get
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1054#issuecomment-213413569
@wido I just want to verify that the standard CI run does not have any
issues. However, @bhaisaab does bring up some good points. We should try to
fix this
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1375#issuecomment-213415473
@milamberspace can you close and reopen this against the 4.8 branch. It is
currently opened against master. Thanks...
---
If your project is set up for it, you
Github user swill commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1376#discussion_r60733737
--- Diff: tools/transifex/.tx/config ---
@@ -89,3 +89,41 @@ trans.pt_BR = work-dir/messages_pt_BR.properties
trans.ru_RU = work-dir
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1509#issuecomment-213419572
@dsclose this PR is currently opened against
`apache:4.7.1-RC20160120T2318`. Should this be merged into 4.7 and then we
will forward merge it to 4.8 and master
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1271#issuecomment-213421066
Ok, perfect. Thanks @footplus for confirming. :)
@anshul1886, I just need this PR to be force pushed to kick off jenkins
again to get everything green so
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1502#issuecomment-213421943
Thanks guys. I will try to get this one queued up for CI.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/837#issuecomment-213422548
I am a bit concerned about this CI run. There is a LOT failing which does
not usually fail. We will need to review the CI results and try to
understand/fix the
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/837#issuecomment-213430064
@bvbharatk no problem. :) Thanks for the continued effort testing. I
just wanted to make sure we understood what was going on.
---
If your project is set up for
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1508#issuecomment-213431039
@kiwiflyer if you wouldn't mind posting the results of your tests that
would be appreciated. :)
@insom if this was actually introduced in 4.8, I agree
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1397#issuecomment-213438660
Thanks for all the effort @bhaisaab. :)
---
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 swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1508#issuecomment-213439120
@insom, yes @kiwiflyer is correct. You will need to base your change off
the 4.8 branch and close this PR and open a new PR based off the 4.8 branch.
Thanks for
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1509#issuecomment-213442284
Lets close this PR and open it based off the 4.7 branch. @ustcweizhou's
LGTM vote will still count on the new PR. Lets just reference this PR in the
new PR
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1512#issuecomment-213443134
You opened it against master again...
