Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1363
---
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
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 jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-209446471
@swill LGTM -- passes static analysis and unit tests + code looks good
---
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/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 jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-209222090
I am confused as to the issue at this point. Building the PR using ``mvn
clean install findbugs:check`` passes. It appears that commit reversions have
addressed
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-208504591
So my understand is there was a FindBugs issue originally. In the process
of fixing the issue, some logic was changed to introduce an issue. This commit
is to
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-208475514
@jburwell I am not saying FindBugs issues should be suppressed in general.
As part of fixing FindBugs issues some functionality got regressed (refer PR
Github user jburwell commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-208410974
@koushik-das I am a hard -1 to a ``@SupressWarnings`` because it will never
get fixed. Was the FindBugs issue present before this patch? If so, it would
be
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-208405525
How about suppressing the findbug issue (using something like
@SuppressWarnings) till someone comes up with a proper fix? In that case there
won't be any mails
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-206935772
Hmmm, not sure how I feel about this. My inbox can't handle a whole bunch
of new findbugs issues. :)
@remibergsma would you mind giving me some guidance on
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-206785324
@swill @ustcweizhou has a point. Id say merge. I'm not going to take the
time to do this now either. One point to consider is that it will up the number
of
Github user ustcweizhou commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-206710882
@swill sorry I will not, that is another issue.
---
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 a diff in the pull request:
https://github.com/apache/cloudstack/pull/1363#discussion_r58813816
--- Diff:
api/src/org/apache/cloudstack/api/command/user/vm/UpgradeVMCmd.java ---
@@ -77,7 +80,18 @@ public Long getServiceOfferingId() {
}
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1363#discussion_r58787775
--- Diff:
api/src/org/apache/cloudstack/api/command/user/vm/UpgradeVMCmd.java ---
@@ -77,7 +80,18 @@ public Long getServiceOfferingId() {
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-206466138
Ok, I understand what is going on here better. @DaanHoogland would you
mind reviewing this for us? Thanks...
---
If your project is set up for it, you can reply
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-206461710
@swill As mentioned by @ustcweizhou in the first comment. I am not sure if
the current scale VM tests cover the dynamic offering scenario.
"This
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-206438944
@koushik-das Can you specify which functionality tests were broken by which
PR? Thanks...
---
If your project is set up for it, you can reply to this email and
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-206221768
+1 based on the code review (reverts 2 commits) and based on the test
results posted by @bvbharatk (no failed tests).
Since existing functionality got
[mailto:sanjeev.neelar...@accelerite.com]
Sent: Wednesday, March 23, 2016 3:45 PM
To: dev@cloudstack.apache.org
Subject: RE: [GitHub] cloudstack pull request: CLOUDSTACK-9251: Fix issue in
scale VM to...
Hi,
I have looked at the test failure from test_04_extract_template test case which
is mentioned
-9251: Fix issue in scale
VM to...
Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-199402183
### ACS CI BVT Run
**Sumarry:**
Build Number 115
Hypervisor xenserver
NetworkType Advanced
Passed=105
Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-199402183
### ACS CI BVT Run
**Sumarry:**
Build Number 115
Hypervisor xenserver
NetworkType Advanced
Passed=105
Failed=11
Skipped=4
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-177145863
The commits that get reverted here were supposed to fix a findbugs issue.
Will this reintroduce them? Can we both fix it and resolve the findbugs issue?
Ping
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 ustcweizhou commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-176945786
@alexandrelimassantana I agree with you about the code changes.
However, this commit only reverts the commit 9c4162a and 16baa12 to fix the
issue.
Github user pdube commented on the pull request:
https://github.com/apache/cloudstack/pull/1363#issuecomment-175089677
Code 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 project does not have this feature
25 matches
Mail list logo
