Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cloudstack.apache.org
From: lsimons <git@git.apache.org>
To: dev@cloudstack.apache.org
Reply-To: dev@cloudstack.apache.org
References: <git-pr-33-cloudstack@git.apache.org>
In-Reply-To: <git-pr-33-cloudstack@git.apache.org>
Subject: [GitHub] cloudstack pull request: Feature cenik123 vpcvrr 1.1.1
Content-Type: text/plain
Message-Id: <20141202085005.E18439B5FA7@tyr.zones.apache.org>
Date: Tue,  2 Dec 2014 08:50:05 +0000 (UTC)

Github user lsimons commented on the pull request:

    https://github.com/apache/cloudstack/pull/33#issuecomment-65199170
  
    Hey folks,
    
    Karl thanks for contributing. Looks like quite a bit of work!
    
    Rohit thanks for CC-ing me in :)
    
    It's a bit hard to read the pull request right now and provide review comments:
    * Github says there's nearly 7000 changed files so at a guess something went wrong there? I had a look, and it looks like KarlHarrisSungardAS/cloudstack is quite a ways behind master, which may explain that...? It's probably a good idea to make a copy of the feature branch, rebase it against current master, squash some of the commits together, and then re-send the pull request. Or, start another branch, and cherry-pick over just the changes that _should_ be in the change set.
    * git hasn't picked up on the relation between veewee and packer, it's probably good idea to give it some more hints. For example it would be good to see
    https://github.com/KarlHarrisSungardAS/cloudstack/commit/892b11ec0230ce87d14370fd1055ac223096cc77#diff-67bdab8b08f1e110ceabe1a23c3d4011R1
    compared to the veewee equivalent.
    * there's a lot of 'shuffling' and/or adding comments intermixed with functional changes. For example https://github.com/KarlHarrisSungardAS/cloudstack/commit/7f55a1ef032c29cb955e02c90da43c91739108d7#diff-95748c84d8b6be2da502091e69b748d9L30 you might think the 'set -x' is getting removed here, but it just moved down a bit amidst a few new comments.
    * there's a lot of commits that are seemingly unrelated to the actual change (probably cherry-picked from master onto the feature branch?), those will need to be filtered out I think.
    * there's a lot of 'archive' commits which have unfinished 'checkpoint' code. I imagine all that work gets finished 'later on' (like renaming _gitignore to .gitignore is one that I tracked) but it's not so easy to see. It'd be great if those things could be squashed together.
    
    As far as content goes...
    
    1) Like sebastien says, veewee is in maintenance mode and moving to packer (replacing veewee) is definitely a good idea. This looks like a straightforward port, which if it works ok seems like the way to go. We _should_ make sure to do it in a way that's somewhat compatible with the jenkins.buildacloud.org setup. I don't think we could sustain maintenance of both packer and veewee builds for long, it'll be a pain to keep changes in sync. I'd want to drop veewee use completely.
    
    2) Testing the systemvm using vagrant is also obviously a good idea (we're doing much the same, over at https://github.com/schubergphilis/cloudstack/tree/feature/systemvm-persistent-config/tools/vagrant/systemvm see https://github.com/schubergphilis/cloudstack/commits/feature/systemvm-test for an abandoned branch having just test-related commits), though I don't understand why we'd need to use vagrant templates? The .box file that we get out of the veewee-based systemvm build imports (https://github.com/apache/cloudstack/blob/master/tools/appliance/build.sh#L466) just fine...
    
    3) For the stuff that seems to be planned _beyond_ these changes, it might be a good idea to pop by on the development mailing list and discuss plans. A few people at Schuberg Philis (my employer, where there's a few people working on cloudstack) have been working on redundancy for the virtual router for a few months now. We've found that making it work also involves a lot of changes outside of the systemvm, too, i.e. changing the orchestration logic in the java code to match changed behavior of the vpc router (see https://cwiki.apache.org/confluence/display/CLOUDSTACK/Refactor+for+Redundant+Virtual+Router+Implementation / https://www.slideshare.net/LeoSimons1/toolkit-posterccceu14-v2 ). @spark404 please do have a look at Karl's approach :)
    
    cheers,
    
    Leo


---
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.
---