i answers something similar to Rafael's answer here, on the PR itself. On Fri, Jan 12, 2018 at 7:21 PM, Rafael Weingärtner < rafaelweingart...@gmail.com> wrote:
> I believe there is no problem in merging Wido’s and Mike’s PRs, they have > been extensively discussed and improved (specially Mike’s one). > > I noticed the merge of #2152 today morning, but crying over spilled milk > does not help anything… The code seems to be ok, maybe those variable names > `cmd` and `cmd2` could benefit from something more descriptive; also, the > magic number `1024`. > > Having said that; I would be ok with it (no need to revert it), but we need > to be more careful with these things. If one wants to merge something, > there is no harm in waiting and calling for reviewers via Github, Slack, or > even email them directly. > > On Fri, Jan 12, 2018 at 3:57 PM, Rohit Yadav <rohit.ya...@shapeblue.com> > wrote: > > > All, > > > > > > We're down to one feature PR towards 4.11 milestone now: > > > > https://github.com/apache/cloudstack/pull/2298 > > > > > > The config drive PR from Frank (Nuage) has been accepted today after no > > regression test failures seen from yesterday's smoketest run. We've also > > tested, reviewed and merge Wido's (blocker fix) PR. > > > > > > I've asked Mike to stabilize the branch; based on the smoketest results > > from today we can see some failures caused by the PR. I'm willing to work > > with Mike and others to get this PR tested, and merged over the weekends > if > > we can demonstrate that no regression is caused by it, i.e. no new > > smoketest regressions. I'll also try to fix regression and test failures > > over the weekend. > > > > > > Lastly, I would like to discuss a mistake I made today with merging the > > following PR which per our guideline lacks one code review lgtm/approval: > > > > https://github.com/apache/cloudstack/pull/2152 > > > > > > The changes in above (merged) PR are all localized to a xenserver-swift > > file, that is not tested by Travis or Trillian, since no new regression > > failures were seen I accepted and merge it on that discretion. The PR was > > originally on the 4.11 milestone, however, due to it lacking a JIRA id > and > > no response from the author it was only recently removed from the > milestone. > > > > > > Please advise if I need to revert this, or we can review/lgtm it > > post-merge? I'll also ping on the above PR. > > > > > > - Rohit > > > > <https://cloudstack.apache.org> > > > > > > > > ________________________________ > > From: Wido den Hollander <w...@widodh.nl> > > Sent: Thursday, January 11, 2018 9:17:26 PM > > To: dev@cloudstack.apache.org > > Subject: Re: [DISCUSS] Freezing master for 4.11 > > > > > > > > On 01/10/2018 07:26 PM, Daan Hoogland wrote: > > > I hope we understand each other correctly: No-one running an earlier > > > version then 4.11 should miss out on any functionality they are using > > now. > > > > > > So if you use ipv6 and multiple cidrs now it must continue to work with > > no > > > loss of functionality. see my question below. > > > > > > On Wed, Jan 10, 2018 at 7:06 PM, Ivan Kudryavtsev < > > kudryavtsev...@bw-sw.com> > > > wrote: > > > > > >> Daan, yes this sounds reasonable, I suppose who would like to fix, > could > > >> do custom build for himself... > > >> > > >> But still it should be aknowledged somehow, if you use several cidrs > for > > >> network, don't use v6, or don't upgrade to 4.11 because things will > stop > > >> running well. > > >> > > > Does this mean that several cidrs in ipv6 works in 4.9 and not in > 4.11? > > > > > > > No, it doesn't. IPv6 was introduced in 4.10 and this broke in 4.10. > > > > You can't run with 4.10 with multiple IPv4 CIDRs as well when you have > > IPv6 enabled. > > > > So this is broken in 4.10 and 4.11 in that case. > > > > Wido > > > > > > > > if yes; it is a blocker > > > > > > if no; you might as well upgrade for other features as it doesn't work > > now > > > either. > > > > > > > rohit.ya...@shapeblue.com > > www.shapeblue.com > > 53 Chandos Place, Covent Garden, London WC2N 4HSUK > > @shapeblue > > > > > > > > > > > -- > Rafael Weingärtner > -- Daan
