CI is running. Thx... :) On May 20, 2016 11:11 PM, "Tutkowski, Mike" <mike.tutkow...@netapp.com> wrote:
Hi, Please feel free to review the following PR that should address this matter by reverting certain variable names back the way they were prior to PR #816: https://github.com/apache/cloudstack/pull/1556 Thanks! Mike ________________________________________ From: Will Stevens <williamstev...@gmail.com> Sent: Friday, May 20, 2016 5:30 PM To: dev@cloudstack.apache.org Subject: Re: Variable renaming in classes meant for Agents Unless that PR was not already accounted for in a grandfathered exception. On May 20, 2016 7:26 PM, "Daan Hoogland" <daan.hoogl...@gmail.com> wrote: > In the mutiny PR I had to change the names that I put final in front so > that implies line level. > > send by tablet, no warranties in existence > On 21 May 2016 01:19, "Will Stevens" <williamstev...@gmail.com> wrote: > > > Does it take offense at a file level or a line of code level? If it is at > > the file level, then it makes sense because people follow the standard > > presented in the file and that file is already accounted for in the > checker > > threshold. > > On May 20, 2016 7:06 PM, "Daan Hoogland" <daan.hoogl...@gmail.com> > wrote: > > > > Concerning the _s, checkstyle takes offence of those in changed lines of > > code. So i am wondering how people can get new _s in. > > > > send by tablet, no warranties in existence > > On 21 May 2016 00:55, "Will Stevens" <wstev...@cloudops.com> wrote: > > > > > I can confirm we currently have Zero HyperV tests in CI. Once we have > > more > > > people contributing to CI we can try to get better coverage, but right > > now > > > I am pretty much just testing on KVM. I know the accelerite guys are > > > testing on Xen. Once I freeze, I will try to do testing of master in > as > > > many setups as I can, but I can't do that all the time because I just > > don't > > > have the bandwidth (in terms of my time) to be able to do that. > > > > > > Our system is far from perfect right now, but I am slowly trying to > close > > > that gap... > > > > > > *Will STEVENS* > > > Lead Developer > > > > > > *CloudOps* *| *Cloud Solutions Experts > > > 420 rue Guy *|* Montreal *|* Quebec *|* H3J 1S6 > > > w cloudops.com *|* tw @CloudOps_ > > > > > > On Fri, May 20, 2016 at 12:49 PM, Tutkowski, Mike < > > > mike.tutkow...@netapp.com > > > > wrote: > > > > > > > Also, does this mean that we have zero Hyper-V integration tests run > > > > during CI? > > > > ________________________________________ > > > > From: Tutkowski, Mike <mike.tutkow...@netapp.com> > > > > Sent: Friday, May 20, 2016 10:47 AM > > > > To: dev@cloudstack.apache.org > > > > Subject: Re: Variable renaming in classes meant for Agents > > > > > > > > Yeah, it has to go into 4.9. :) Unless no one cares about Hyper-V. > > > > ________________________________________ > > > > From: Rafael Weingärtner <rafaelweingart...@gmail.com> > > > > Sent: Friday, May 20, 2016 10:42 AM > > > > To: dev@cloudstack.apache.org > > > > Subject: Re: Variable renaming in classes meant for Agents > > > > > > > > You are right Mike about the “_”. The point is that in some other > > > language > > > > the use of “_” makes sense, whereas in Java it does not, at least not > > the > > > > way it has being used in ACS. > > > > > > > > We have code conventions, it can be found in [1]. The problem is that > > it > > > is > > > > a bit outdated and I think it could benefit from some others > tutorials. > > > For > > > > instance, a clear and simple tutorial explaining what is a test case > > and > > > > showing how to create a proper test case; I am referring to the > > > > unit/integration test case that we write using Junit and other tools. > > > > > > > > Also, we lack some information on how to prepare code to be tested. > > > > Once we have that kind of standard defined and tutorials written, we > > > could > > > > work out ways to educate our community. It is not a problem not to > know > > > > those things; we cannot expect everyone to know how to use every > single > > > > methodology and technology that is out there. But, we can help people > > to > > > > learn, that is the point of a community, it should be a place where > > > people > > > > exchange ideas and experience in a way that benefits everyone. > > > > > > > > As soon as you open the PR, please let us know, so we can review it > and > > > > help you get it merge as soon as possible. I think this is something > > that > > > > should go in the 4.9 release. > > > > > > > > > > > > On Fri, May 20, 2016 at 1:27 PM, Tutkowski, Mike < > > > > mike.tutkow...@netapp.com> > > > > wrote: > > > > > > > > > It sounds like most people don't like a preceding "_" for member > > > > variables > > > > > and that is fine. Do we have any formal Java coding standards for > > > > > CloudStack, by the way? I'm not aware of any. > > > > > > > > > > The main problem here, though, is that this particular piece of > code > > is > > > > > super fragile, so it would be great to harden it up. > > > > > > > > > > I'm going to open a PR and revert the names in those changed > > "Command" > > > > > files for 4.9. That will solve the immediate problem. > > > > > ________________________________________ > > > > > From: Rafael Weingärtner <rafaelweingart...@gmail.com> > > > > > Sent: Friday, May 20, 2016 9:12 AM > > > > > To: dev@cloudstack.apache.org > > > > > Subject: Re: Variable renaming in classes meant for Agents > > > > > > > > > > Hi guys, > > > > > I agree with Daan that if class fields have improper (not > descriptive > > > or > > > > > not suitable) names we should change them. However, I do not find > the > > > > > change (on variable names) introduced by PR #816 good. It added an > > > > > “_”(underline) before variable names; even though Apache CloudStack > > > has a > > > > > lot of that currently, I think that is a pattern we should avoid. > > > > > > > > > > Your ideas to use annotations to avoid relying on variable names > are > > > > great; > > > > > but, let’s not re-create the well here. There is a research [1] > that > > > has > > > > > been conducted in 2014 that tackled exactly that problem; the > > proposal > > > > > presented in [1] decoupled client and server sides from variable > name > > > by > > > > > using semantic annotations. The concept, the formalization and the > > > > > experiments are all presented in paper [1]. The serialization and > > > > > deserialization core of the proposal presented in [1] can be found > in > > > > [2]. > > > > > > > > > > The idea of decoupling our web APIs from variable names is great, > but > > > it > > > > is > > > > > something that will require some effort to be fully and properly > > > > > implemented. If you wish to push that forward count on me. > > > > > > > > > > [1] > > http://ieeexplore.ieee.org/xpls/abs_all.jsp?arnumber=6928953&tag=1 > > > > > [2] https://github.com/ivansalvadori/gsonld > > > > > > > > > > > > > > > On Fri, May 20, 2016 at 3:30 AM, Daan Hoogland < > > > daan.hoogl...@gmail.com> > > > > > wrote: > > > > > > > > > > > Guys, we should rename fields that have improper names. I do not > > > agreee > > > > > > with the statement at all. Your two solutions to the > serialisation > > > > hazard > > > > > > are both acceptable to me. leaving a non compliant or non > > explanatory > > > > > name > > > > > > in because it slipped through the nets at some points does not > seem > > > > > > acceptable to me. We must improve are code. > > > > > > > > > > > > On Fri, May 20, 2016 at 6:53 AM, Tutkowski, Mike < > > > > > > mike.tutkow...@netapp.com> > > > > > > wrote: > > > > > > > > > > > > > Thanks for sending out this e-mail, Anshul. > > > > > > > > > > > > > > This is a bit of a strange situation because we need to make > sure > > > > > people > > > > > > > are either aware of the fact that properties in Command classes > > are > > > > > > > serialized (and not change existing variable names) or come up > > > with a > > > > > > less > > > > > > > fragile way of choosing property names when sending data > (perhaps > > > > using > > > > > > > annotations). > > > > > > > > > > > > > > At the very least, we should have comments in these classes > > > > indicating > > > > > > the > > > > > > > dangers of changing property names. It might also be beneficial > > to > > > > have > > > > > > > unit tests in place that expect certain variable names and > assert > > > if > > > > > they > > > > > > > are not as expected. > > > > > > > > > > > > > > In the meanwhile, I plan to change the variable names back that > > > were > > > > > > > changed in PR #816. > > > > > > > > > > > > > > Additional thoughts on how this should be addressed long term? > > > > > > > > > > > > > > Thanks! > > > > > > > Mike > > > > > > > ________________________________________ > > > > > > > From: Anshul Gangwar <anshul.gang...@accelerite.com> > > > > > > > Sent: Thursday, May 19, 2016 10:47 PM > > > > > > > To: dev@cloudstack.apache.org > > > > > > > Subject: Variable renaming in classes meant for Agents > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > We should not allow renaming of variables in classes which ends > > > with > > > > > > > Command and TO. As these objects are meant to be consumed by > > > Agents. > > > > > > > > > > > > > > Agents may not be written in java so relying on these variable > > > names > > > > to > > > > > > > get the info. One such example is Hyper-V agent. > > > > > > > > > > > > > > Hyper-V support is currently broken as there are some variables > > > > renamed > > > > > > in > > > > > > > PR https://github.com/apache/cloudstack/pull/816. > > > > > > > > > > > > > > Regards, > > > > > > > Anshul > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > DISCLAIMER > > > > > > > ========== > > > > > > > This e-mail may contain privileged and confidential information > > > which > > > > > is > > > > > > > the property of Accelerite, a Persistent Systems business. It > is > > > > > intended > > > > > > > only for the use of the individual or entity to which it is > > > > addressed. > > > > > If > > > > > > > you are not the intended recipient, you are not authorized to > > read, > > > > > > retain, > > > > > > > copy, print, distribute or use this message. If you have > received > > > > this > > > > > > > communication in error, please notify the sender and delete all > > > > copies > > > > > of > > > > > > > this message. Accelerite, a Persistent Systems business does > not > > > > accept > > > > > > any > > > > > > > liability for virus infected mails. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Daan > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Rafael Weingärtner > > > > > > > > > > > > > > > > > > > > > -- > > > > Rafael Weingärtner > > > > > > > > > >
