You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by borisroman <gi...@git.apache.org> on 2015/08/21 14:06:37 UTC
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
GitHub user borisroman opened a pull request:
https://github.com/apache/cloudstack/pull/726
Fix for the NicVO.java regression.
Renamed set*() methods to correct naming.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/borisroman/cloudstack NicVORegression
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/726.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #726
----
commit 4b88eabef1310fde8631cc3874a5e07dbca5ee4c
Author: Boris Schrijver <bo...@pcextreme.nl>
Date: 2015-08-21T08:06:12Z
Fix for the NicVO.java regression.
Renamed set*() methods to correct naming.
----
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on the pull request:
https://github.com/apache/cloudstack/pull/726#issuecomment-133408184
@karuturi I came to that conclusion in the previous pr. I will stack all refactoring work in a branch. And send a PR when done.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/726#issuecomment-133567885
hm, you shouldt merge anything to a broken master that isn't hellbound on fixing it.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/726#issuecomment-133570169
@DaanHoogland It does fix the problem, I just feel a bit blind sometimes by the builds that seem less reliable lately so then it's hard to be sure.
But anyway, when looking back we should have quickly reverted the initial PR that introduced the problem. Next time we should do that. Benefit is master is stable again, plus there is time to work on a fix that can then be send as a PR along with the original commits.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on the pull request:
https://github.com/apache/cloudstack/pull/726#issuecomment-133398943
Tested it in a new environment. The nics are correctly put in the db 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 your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on the pull request:
https://github.com/apache/cloudstack/pull/726#issuecomment-133395038
The updateBuilder will check if the DAO has changed based on the set*() methods. When it can't resolve one, it won't update. After learning this I changed the set*() method names to reflect the names of the variable fields.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/726#issuecomment-133544954
LGTM Verified that a new cloud deployment had working system VMs 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 your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/726#issuecomment-133397841
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/726#issuecomment-133567102
Check, as far as I can tell everything is OK. And since master is broken now anyway, I think it's time to merge this to fix it.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/726#issuecomment-133546545
cloudstack-pull-analysis 296 failed due to: `Expected exactly 2 but it took more than 3 microseconds between 2 consecutive calls to System.nanoTime().` Seems not related.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/726#issuecomment-133552422
I think we have to disable this test, enlarging the period even more beats every intention of its use. not related indeed
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/726
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/726#issuecomment-133467580
@borisroman Thanks! I'll run some tests later tonight. Travis is green, didn't see an Apache build reporting results (yet?).
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on the pull request:
https://github.com/apache/cloudstack/pull/726#issuecomment-133399200
@remibergsma @karuturi Please review!
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix for the NicVO.java regression.
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/726#issuecomment-133406706
LGTM. lets wait for travis and other jenkins verifications.
In future, can you club all refactoring work in a branch and merge them together? That way, we can test it once(giving bigger time window for everyone to verofy) and merge
Also, I think this refactoring is a candidate for 4.7. We are already close to RC on 4.6(once 4.5.2 is done) and cannot afford regressions at this stage.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---