Re: [01/50] git commit: updated refs/heads/master to 1290e10

2014-09-24 Thread Leo Simons 
Hey hey,

On Sep 23, 2014, at 11:50 PM, David Nalley da...@gnsa.us wrote:
 On Tue, Sep 23, 2014 at 4:44 PM, Rohit Yadav rohit.ya...@shapeblue.com 
 wrote:
 Hi David,
 
 On 23-Sep-2014, at 8:31 pm, David Nalley da...@gnsa.us wrote:
 Where was the merge request for this huge merge to master? (it was at
 50 commit emails, when it stopped sending, )
 We have passed feature freeze for 4.5.0, so I am confused as why this
 was merged. Is there a reason not to revert all of this?
 
 This was the request: https://github.com/apache/cloudstack/pull/16
 And JIRA: https://issues.apache.org/jira/browse/CLOUDSTACK-7143
 We all get emails from Github PR (just re-checked) so you may find them on 
 the ML.
 
 Yes, GH PR is exactly like the Review Board emails in this particular
 aspect. My question is why is this merged into master rather than a
 feature branch, and why no [MERGE] email as per:
 https://cwiki.apache.org/confluence/display/CLOUDSTACK/Branch+Merge+Expectations

Hmm, I hadn’t seen that before; don’t think it’s linked from the “how to 
contribute” stuff. Sorry. Or is this something a committer is to do?

Can we change the rules? Because I’d guess the [MERGE] e-mail serves exactly 
the same role as a pull request: widely announcing a branch is ready to merge 
in and providing a trigger point for discussion.

It’s also interesting to consider whether you need a feature branch at all in 
the local repository for branches that were first developed (and tested) 
remotely. After all the only remaining thing you will do to that branch is to 
merge it in. The original feature branch is

  https://github.com/schubergphilis/CloudStack/tree/feature/systemvm-refactor

which has existed for a few months and was discussed here a few times (well I 
sent e-mail about it, there wasn’t much discussion…), with

  
https://github.com/schubergphilis/CloudStack/tree/feature/systemvm-refactor-for-upstream

as the re-re-rebased version of all that. This is all github standard (best?) 
practice, to the point that you cannot even create pull requests that result in 
the creation of new branches. I.e. if you look at

  
https://github.com/schubergphilis/cloudstack/compare/feature/systemvm-refactor-for-upstream

and you click the “edit” in the top row, you can’t even suggest merging to a 
new branch, there’s only existing ones listed. Of course the committer doing 
the actual upstreaming can make a different choice, but, why would you?

As for whether to take this for 4.5.0, of course the risk is not 0, but its 
pretty darn low. I also think by typical community rules just about none of the 
commits in the pull request are a new feature. Longer term the point of these 
changes is a quality increase by being just slightly more precise and 
deliberate in a few places. If you think they don’t accomplish that you should 
definitely pull it out, but then I’d like to hear what’s wrong!

If you pull it out, please do consider something more limited like

https://github.com/schubergphilis/cloudstack/commit/74c381540ebaf940bbbf471b580303488aa9c5b4
which avoids putting master version of the systemvm code into branch builds.

I would also _really_ like to see a virtualbox systemvm.box out of the official 
4.5.0 build so that we can easily use our systemvm component tests for 
regression testing the changes we’ll have for 4.5.1/4.6.0. If you back this out 
it’ll be annoying bit twiddling to make one manually.



cheers!


Leo

Re: [01/50] git commit: updated refs/heads/master to 1290e10

2014-09-23 Thread Hugo Trippaers 
Hey David,

This is one of the requests that came in using the “new github pull request 
thing. The big advantage is that we leverage the nice things from github. Part 
of doing it that way means we keep the history of the original developer 
intact. With review board we typically get one smashed commit with the entire 
change. This is an entire commit history we got working up to a rewrite of the 
build scripts for the systemvm. I’m not exactly sure if that is what we want 
from github yet, but lets see what we all think about it. Procedurally this is 
the same as merging something in from the review board after review.

As for the content, i’m pretty biased as it is part of the ongoing project to 
introduce the redundant VPC router. Leo did a great job in rewriting the build 
scripts for the systemvms. No changing any functionality, but mainly making the 
code and scripts more accessible. I’m thinking of this as a change to packaging 
more than merging new features. There are changes pending that will change 
functionality, but those are planned for after 4.5 happens.

Cheers,

Hugo



On 23 sep. 2014, at 20:31, David Nalley da...@gnsa.us wrote:

 Where was the merge request for this huge merge to master? (it was at
 50 commit emails, when it stopped sending, )
 We have passed feature freeze for 4.5.0, so I am confused as why this
 was merged. Is there a reason not to revert all of this?
 
 --David
 
 On Mon, Sep 22, 2014 at 3:44 PM,  bhais...@apache.org wrote:
 Repository: cloudstack
 Updated Branches:
  refs/heads/master a6ee4112a - 1290e1010
 
 
 CLOUDSTACK-7143: move fix_acpid to its own file
 
 
 Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo
 Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/5627b67f
 Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/5627b67f
 Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/5627b67f
 
 Branch: refs/heads/master
 Commit: 5627b67ff3a6af70949ee1622b3e5a572d39a0b7
 Parents: 6a688a0
 Author: Leo Simons lsim...@schubergphilis.com
 Authored: Mon Jul 21 11:19:03 2014 +0200
 Committer: Rohit Yadav rohit.ya...@shapeblue.com
 Committed: Mon Sep 22 21:31:35 2014 +0200
 
 --
 .../definitions/systemvmtemplate/configure_acpid.sh  | 15 +++
 .../definitions/systemvmtemplate/definition.rb   |  1 +
 .../definitions/systemvmtemplate/postinstall.sh  | 15 ---
 3 files changed, 16 insertions(+), 15 deletions(-)
 --
 
 
 http://git-wip-us.apache.org/repos/asf/cloudstack/blob/5627b67f/tools/appliance/definitions/systemvmtemplate/configure_acpid.sh
 --
 diff --git a/tools/appliance/definitions/systemvmtemplate/configure_acpid.sh 
 b/tools/appliance/definitions/systemvmtemplate/configure_acpid.sh
 new file mode 100644
 index 000..70abe30
 --- /dev/null
 +++ b/tools/appliance/definitions/systemvmtemplate/configure_acpid.sh
 @@ -0,0 +1,15 @@
 +fix_acpid() {
 +  # Fix acpid
 +  mkdir -p /etc/acpi/events
 +  cat  /etc/acpi/events/power  EOF
 +event=button/power.*
 +action=/usr/local/sbin/power.sh %e
 +EOF
 +  cat  /usr/local/sbin/power.sh  EOF
 +#!/bin/bash
 +/sbin/poweroff
 +EOF
 +  chmod a+x /usr/local/sbin/power.sh
 +}
 +
 +fix_acpid
 
 http://git-wip-us.apache.org/repos/asf/cloudstack/blob/5627b67f/tools/appliance/definitions/systemvmtemplate/definition.rb
 --
 diff --git a/tools/appliance/definitions/systemvmtemplate/definition.rb 
 b/tools/appliance/definitions/systemvmtemplate/definition.rb
 index be0b403..a2eb82b 100644
 --- a/tools/appliance/definitions/systemvmtemplate/definition.rb
 +++ b/tools/appliance/definitions/systemvmtemplate/definition.rb
 @@ -63,6 +63,7 @@ config = {
 'configure_locale.sh',
 'configure_login.sh',
 'postinstall.sh',
 +'configure_acpid.sh',
 'cleanup.sh',
 'configure_networking.sh',
 'zerodisk.sh'
 
 http://git-wip-us.apache.org/repos/asf/cloudstack/blob/5627b67f/tools/appliance/definitions/systemvmtemplate/postinstall.sh
 --
 diff --git a/tools/appliance/definitions/systemvmtemplate/postinstall.sh 
 b/tools/appliance/definitions/systemvmtemplate/postinstall.sh
 index 893b521..f2ce1ae 100644
 --- a/tools/appliance/definitions/systemvmtemplate/postinstall.sh
 +++ b/tools/appliance/definitions/systemvmtemplate/postinstall.sh
 @@ -116,20 +116,6 @@ nameserver 8.8.4.4
 EOF
 }
 
 -fix_acpid() {
 -  # Fix acpid
 -  mkdir -p /etc/acpi/events
 -  cat  /etc/acpi/events/power  EOF
 -event=button/power.*
 -action=/usr/local/sbin/power.sh %e
 -EOF
 -  cat  /usr/local/sbin/power.sh  EOF
 -#!/bin/bash
 -/sbin/poweroff
 -EOF
 -  chmod a+x /usr/local/sbin/power.sh
 -}
 -
 fix_hostname() {
   # Fix

Re: [01/50] git commit: updated refs/heads/master to 1290e10

2014-09-23 Thread Rohit Yadav 
Hi David,

On 23-Sep-2014, at 8:31 pm, David Nalley da...@gnsa.us wrote:
 Where was the merge request for this huge merge to master? (it was at
 50 commit emails, when it stopped sending, )
 We have passed feature freeze for 4.5.0, so I am confused as why this
 was merged. Is there a reason not to revert all of this?

This was the request: https://github.com/apache/cloudstack/pull/16
And JIRA: https://issues.apache.org/jira/browse/CLOUDSTACK-7143
We all get emails from Github PR (just re-checked) so you may find them on the 
ML.

I was reviewing this Github pull request which came in couple of weeks ago. 
This is well tested and successfully refactors the way we build systemvms. It 
was only merged when it passed build tests and painful manual QA (I did on 
KVM). Just want to mention that this is not my code or $dayjob work, I was just 
trying to help out the contribution from community member(s).

Pardon my ignorance but I was not aware about our official status of 
master/4.5.0 that we’re already passed feature freeze. Can you confirm the 
official stand/status on master/4.5.0 and when do we plan to cut the release 
branch, and if we are changing anything with the way we do releases etc?

I see a lot of changes that comes in every other day on master. The pattern I 
see is that developers check-in barely working feature and then find the excuse 
to check-in more patches/commits as bug fixes where they actually are trying to 
complete an incomplete feature. I think this is a chronic issue which should be 
checked and everyone should make an effort to work in their feature branch, 
work on their git-foo, rebase/fix-conflicts and send merge request or Github 
pull requests or request using reviewboard. I think this will allow all of us 
to participate in on-going efforts and would help improve its quality using QA 
and code reviewing.

Regards,
Rohit Yadav
Software Architect, ShapeBlue
M. +41 779015219 | rohit.ya...@shapeblue.com
Blog: bhaisaab.org | Twitter: @_bhaisaab

Find out more about ShapeBlue and our range of CloudStack related services

IaaS Cloud Design  Buildhttp://shapeblue.com/iaas-cloud-design-and-build//
CSForge – rapid IaaS deployment frameworkhttp://shapeblue.com/csforge/
CloudStack Consultinghttp://shapeblue.com/cloudstack-consultancy/
CloudStack Infrastructure 
Supporthttp://shapeblue.com/cloudstack-infrastructure-support/
CloudStack Bootcamp Training Courseshttp://shapeblue.com/cloudstack-training/

This email and any attachments to it may be confidential and are intended 
solely for the use of the individual to whom it is addressed. Any views or 
opinions expressed are solely those of the author and do not necessarily 
represent those of Shape Blue Ltd or related companies. If you are not the 
intended recipient of this email, you must neither take any action based upon 
its contents, nor copy or show it to anyone. Please contact the sender if you 
believe you have received this email in error. Shape Blue Ltd is a company 
incorporated in England  Wales. ShapeBlue Services India LLP is a company 
incorporated in India and is operated under license from Shape Blue Ltd. Shape 
Blue Brasil Consultoria Ltda is a company incorporated in Brasil and is 
operated under license from Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company 
registered by The Republic of South Africa and is traded under license from 
Shape Blue Ltd. ShapeBlue is a registered trademark.

Re: [01/50] git commit: updated refs/heads/master to 1290e10

2014-09-23 Thread David Nalley 
On Tue, Sep 23, 2014 at 4:44 PM, Rohit Yadav rohit.ya...@shapeblue.com wrote:
 Hi David,

 On 23-Sep-2014, at 8:31 pm, David Nalley da...@gnsa.us wrote:
 Where was the merge request for this huge merge to master? (it was at
 50 commit emails, when it stopped sending, )
 We have passed feature freeze for 4.5.0, so I am confused as why this
 was merged. Is there a reason not to revert all of this?

 This was the request: https://github.com/apache/cloudstack/pull/16
 And JIRA: https://issues.apache.org/jira/browse/CLOUDSTACK-7143
 We all get emails from Github PR (just re-checked) so you may find them on 
 the ML.


Yes, GH PR is exactly like the Review Board emails in this particular
aspect. My question is why is this merged into master rather than a
feature branch, and why no [MERGE] email as per:
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Branch+Merge+Expectations

 I was reviewing this Github pull request which came in couple of weeks ago. 
 This is well tested and successfully refactors the way we build systemvms. It 
 was only merged when it passed build tests and painful manual QA (I did on 
 KVM). Just want to mention that this is not my code or $dayjob work, I was 
 just trying to help out the contribution from community member(s).

 Pardon my ignorance but I was not aware about our official status of 
 master/4.5.0 that we’re already passed feature freeze. Can you confirm the 
 official stand/status on master/4.5.0 and when do we plan to cut the release 
 branch, and if we are changing anything with the way we do releases etc?


See:
http://markmail.org/message/jctcystkzv4sqrvz

Based on that thread, we decided not to branch 4.5.0 and instead keep
master = 4.5.0 until much later in the schedule.
Hence merging features into master is breaking feature freeze.

 I see a lot of changes that comes in every other day on master. The pattern I 
 see is that developers check-in barely working feature and then find the 
 excuse to check-in more patches/commits as bug fixes where they actually are 
 trying to complete an incomplete feature. I think this is a chronic issue 
 which should be checked and everyone should make an effort to work in their 
 feature branch, work on their git-foo, rebase/fix-conflicts and send merge 
 request or Github pull requests or request using reviewboard. I think this 
 will allow all of us to participate in on-going efforts and would help 
 improve its quality using QA and code reviewing.


This is a separate issue; but I tend to agree with your analysis of
how things happen. If you see this happening though, please speak up.
As a committer you have veto power. Speaking frankly, we all have to
be on guard for quality issues.

--David

Re: [01/50] git commit: updated refs/heads/master to 1290e10

2014-09-23 Thread Rohit Yadav 
Hi David,

On 23-Sep-2014, at 11:50 pm, David Nalley da...@gnsa.us wrote:
 Yes, GH PR is exactly like the Review Board emails in this particular
 aspect. My question is why is this merged into master rather than a
 feature branch, and why no [MERGE] email as per:
 https://cwiki.apache.org/confluence/display/CLOUDSTACK/Branch+Merge+Expectations

Ah, my bad. I did not consider the process/syntax around it. There was some 
initial email on this but it did not start with a “[MERGE]” but started with 
“rfc:” for merging it on master:
http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201407.mbox/%3C7A6CF878-7A28-4D4A-BCD2-0C264F8C90B7%40schubergphilis.com%3E

I also forgot that this process should apply for merging changes from 
reviewboard and Github PR as well. That’s completely my fault to not enforce 
the branch merging process on this PR. The only thing I can offer now is to 
make sure that this work won’t break build or anything else on master/4.5.0

Regards,
Rohit Yadav
Software Architect, ShapeBlue
M. +41 779015219 | rohit.ya...@shapeblue.com
Blog: bhaisaab.org | Twitter: @_bhaisaab

Find out more about ShapeBlue and our range of CloudStack related services

IaaS Cloud Design  Buildhttp://shapeblue.com/iaas-cloud-design-and-build//
CSForge – rapid IaaS deployment frameworkhttp://shapeblue.com/csforge/
CloudStack Consultinghttp://shapeblue.com/cloudstack-consultancy/
CloudStack Infrastructure 
Supporthttp://shapeblue.com/cloudstack-infrastructure-support/
CloudStack Bootcamp Training Courseshttp://shapeblue.com/cloudstack-training/

This email and any attachments to it may be confidential and are intended 
solely for the use of the individual to whom it is addressed. Any views or 
opinions expressed are solely those of the author and do not necessarily 
represent those of Shape Blue Ltd or related companies. If you are not the 
intended recipient of this email, you must neither take any action based upon 
its contents, nor copy or show it to anyone. Please contact the sender if you 
believe you have received this email in error. Shape Blue Ltd is a company 
incorporated in England  Wales. ShapeBlue Services India LLP is a company 
incorporated in India and is operated under license from Shape Blue Ltd. Shape 
Blue Brasil Consultoria Ltda is a company incorporated in Brasil and is 
operated under license from Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company 
registered by The Republic of South Africa and is traded under license from 
Shape Blue Ltd. ShapeBlue is a registered trademark.