From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e28smtp03.in.ibm.com (e28smtp03.in.ibm.com [125.16.236.3]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id EEF491A0D83 for ; Tue, 12 Jan 2016 20:37:11 +1100 (AEDT) Received: from localhost by e28smtp03.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 12 Jan 2016 15:07:08 +0530 Received: from d28dlp01.in.ibm.com (9.184.220.126) by e28smtp03.in.ibm.com (192.168.1.133) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 12 Jan 2016 15:07:06 +0530 X-IBM-Helo: d28dlp01.in.ibm.com X-IBM-MailFrom: mkumatag@in.ibm.com X-IBM-RcptTo: openbmc@lists.ozlabs.org Received: from d28relay02.in.ibm.com (d28relay02.in.ibm.com [9.184.220.59]) by d28dlp01.in.ibm.com (Postfix) with ESMTP id C5D74E0068 for ; Tue, 12 Jan 2016 15:07:52 +0530 (IST) Received: from d28av06.in.ibm.com (d28av06.in.ibm.com [9.184.220.48]) by d28relay02.in.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u0C9aJcT17957044 for ; Tue, 12 Jan 2016 15:06:19 +0530 Received: from d28av06.in.ibm.com (localhost [127.0.0.1]) by d28av06.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u0C9aJIG000963 for ; Tue, 12 Jan 2016 15:06:19 +0530 Received: from d50lp31.co.us.ibm.com (d50lp31.boulder.ibm.com [9.17.249.32]) by d28av06.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id u0C9aFdW000730 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL) for ; Tue, 12 Jan 2016 15:06:18 +0530 Message-Id: <201601120936.u0C9aFdW000730@d28av06.in.ibm.com> Received: from localhost by d50lp31.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 12 Jan 2016 02:36:15 -0700 Received: from smtp.notes.na.collabserv.com (192.155.248.82) by d50lp31.co.us.ibm.com (192.168.2.141) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256/256) Tue, 12 Jan 2016 02:36:12 -0700 X-IBM-Helo: smtp.notes.na.collabserv.com X-IBM-MailFrom: mkumatag@in.ibm.com X-IBM-RcptTo: openbmc@lists.ozlabs.org Received: from /spool/local by smtp.notes.na.collabserv.com with smtp.notes.na.collabserv.com ESMTP for from ; Tue, 12 Jan 2016 09:36:11 -0000 Received: from us1a3-smtp04.a3.dal06.isc4sb.com (10.106.154.237) by smtp.notes.na.collabserv.com (10.106.227.105) with smtp.notes.na.collabserv.com ESMTP; Tue, 12 Jan 2016 09:36:08 -0000 Received: from us1a3-mail108.a3.dal06.isc4sb.com ([10.146.45.126]) by us1a3-smtp04.a3.dal06.isc4sb.com with ESMTP id 2016011209360725-107901 ; Tue, 12 Jan 2016 09:36:07 +0000 In-Reply-To: <20160112111038.0915e033@camb691> Subject: Re: [PATCH openbmc-test-automation 00/60] Pull Manjunath's automation repository To: Cyril Bur Cc: OpenBMC Patches , openbmc@lists.ozlabs.org From: "Manjunath A Kumatagi" Date: Tue, 12 Jan 2016 15:05:56 +0530 References: <1452531026-13715-1-git-send-email-openbmc-patches@stwcx.xyz> <20160112111038.0915e033@camb691> X-KeepSent: EB0431E9:C01DA08F-65257F38:00343C5E; type=4; name=$KeepSent X-Mailer: IBM Notes Release 9.0.1FP5 Octobe4, 2013 X-LLNOutbound: False X-Disclaimed: 46831 X-TNEFEvaluated: 1 Content-type: multipart/related; Boundary="0__=EABBF5ABDFA7BACE8f9e8a93df938690918cEABBF5ABDFA7BACE" x-cbid: 16011209-0009-0000-0000-000009E7D902 X-IBM-ISS-SpamDetectors: Score=0.410717; BY=0.133433; FL=0; FP=0; FZ=0; HX=0; KW=0; PH=0; SC=0.410717; ST=0; TS=0; UL=0; ISC= X-IBM-ISS-DetailInfo: BY=3.00004778; HX=3.00000239; KW=3.00000007; PH=3.00000004; SC=3.00000131; SDB=6.00644132; UDB=6.00290366; UTC=2016-01-12 09:36:09 x-cbparentid: 16011209-9900-0000-0000-000005A204FB X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Jan 2016 09:37:12 -0000 --0__=EABBF5ABDFA7BACE8f9e8a93df938690918cEABBF5ABDFA7BACE Content-type: multipart/alternative; Boundary="1__=EABBF5ABDFA7BACE8f9e8a93df938690918cEABBF5ABDFA7BACE" --1__=EABBF5ABDFA7BACE8f9e8a93df938690918cEABBF5ABDFA7BACE Content-Transfer-Encoding: quoted-printable Content-type: text/plain; charset=US-ASCII These code changes are done long back in my private repository, now part of mirroring process into openbmc org - we are seeing all of these big list of commits again in the PR. However all points mention below are very valid observation and I'll make sure to follow those guidelines going forward. Thanks, Manjunath. From: Cyril Bur To: OpenBMC Patches Cc: openbmc@lists.ozlabs.org Date: 01/12/2016 05:41 AM Subject: Re: [PATCH openbmc-test-automation 00/60] Pull Manjunath's automation repository Sent by: "openbmc" On Mon, 11 Jan 2016 10:49:26 -0600 OpenBMC Patches wrote: > Pull Manjunath's repository: https://github.com/mkumatag/openbmc-automation > into openbmc. > Ummmm what happened here? It looks like the entire series is sent twice? I've had an initial skim read and it looks to me like you can squash A LOT of those patches together. The commits don't need to be a story of how you got from no code to the this point in time. Each commit should add a feature and associated documentation. It looks like most of patches 0-6 can be one commit, same for some subsequent patches. Heres an example of some issues I've noticed: In patch 2 you add tests/test=5Fssh.robot but then in patch 7 you delete tests/test=5Fssh.robot. There are several examples of a patch adding a significant chunk of work and then a later patch removing this chunk of work, if after the series the chunk of work won't be in the repo then it shouldn't have appeared in the series at all. Patch 26 "Fix errors" should be squashed into the broken commit. Patch 22 looks that way too and Patch 17. The reason we do this is that way when something does go wrong and something doesn't work it makes it much easier to find the broken patch, you should never submit a patches that is broken even if it is fixed directly after this breaks the assumption that every commit we make is good and therefore when chasing a bug if a broken patch is found, is it likely to be that bug, much easier to automate. Your patch 28 that "Adds one more ssl test" should be squashed into the patch adding the ssl tests, add them all once for this series. You can always more work on more later but this series you have one patch adding ssl tests, have all the ssl tests you're adding in that patch. Hopefully that makes sense, please ask if I've just rambled! Cyril > https://github.com/openbmc/openbmc-test-automation/pull/1 > > Chris Austen (16): > Added DIMM specific inventory tests > base add for sensor tests > fixed review comments > created loops for tests > Add initial rest test cases Support to test Null properties > Added full sensor suite > Update README.md > Handle new rest client from Brad > Added DIMM specific inventory tests > base add for sensor tests > fixed review comments > created loops for tests > Add initial rest test cases Support to test Null properties > Added full sensor suite > Update README.md > Handle new rest client from Brad > > Manjunath A Kumatagi (34): > Added git ignore file > Use argumnet file with pyrobot and added instructions to READEME file. > Update README for better html display > Added Inventory list testcase > fixed errors in tox and generate=5Fargumentfile.sh script > Moved Read Attribute and Read Properties to rest=5Fclient > fix review comment > Added SSL tests > Ran the tests and fixed errors in SSL tests > Modified the doc > Added firmware version validation test > update readme file with tox version > Fix errors > Fix Errors in test=5Fobmcrest.robot and removed the empty testcases. > Added one more testcase to test non-ssl connection to port 443 > OpenBMC REST Testing > updated the REST API for firmware version > Added git ignore file > Use argumnet file with pyrobot and added instructions to READEME file. > Update README for better html display > Added Inventory list testcase > fixed errors in tox and generate=5Fargumentfile.sh script > Moved Read Attribute and Read Properties to rest=5Fclient > fix review comment > Added SSL tests > Ran the tests and fixed errors in SSL tests > Modified the doc > Added firmware version validation test > update readme file with tox version > Fix errors > Fix Errors in test=5Fobmcrest.robot and removed the empty testcases. > Added one more testcase to test non-ssl connection to port 443 > OpenBMC REST Testing > updated the REST API for firmware version > > Manjunath Kumatagi (2): > first commit > first commit > > manjunath (6): > added files > added log statement > added testcase for etc passwd > added files > added log statement > added testcase for etc passwd > > mkumatag (2): > Update README.md > Update README.md > > > =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F= =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F > openbmc mailing list > openbmc@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/openbmc =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F= =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F openbmc mailing list openbmc@lists.ozlabs.org https://lists.ozlabs.org/listinfo/openbmc --1__=EABBF5ABDFA7BACE8f9e8a93df938690918cEABBF5ABDFA7BACE Content-Transfer-Encoding: quoted-printable Content-type: text/html; charset=US-ASCII Content-Disposition: inline

These code changes are done long back in my private reposito= ry, now part of mirroring process into openbmc org - we are seeing all of t= hese big list of commits again in the PR. However all points mention below = are very valid observation and I'll make sure to follow those guidelines go= ing forward.

Thanks,
Manjunath.
3D"InactiveCyril Bur ---01/12/2016 05:41:04 AM---On Mon, 11 Jan 2016 1= 0:49:26 -0600 OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:

From: Cyril Bur <cyrilbur@gmail.com>
To: OpenBMC Patches <openbm= c-patches@stwcx.xyz>
Cc: = openbmc@lists.ozlabs.org
Date: 01/12/2016 05= :41 AM
Subject: = Re: [PATCH openbmc-test-automation 00/60] Pull Manjunath's= automation repository
Sent by= : "openbmc" <openbmc-bounces+mk= umatag=3Din.ibm.com@lists.ozlabs.org>




On = Mon, 11 Jan 2016 10:49:26 -0600
OpenBMC Patches <openbmc-patches@stwc= x.xyz> wrote:

> Pull Manjunath's repository: https://github.com/mkum= atag/openbmc-automation
> into openbmc.
>

= Ummmm what happened here? It looks like the entire series is sent twice?
I've had an initial skim read and it looks to me like you can squash A= LOT of
those patches together.

The commits don't need to be a st= ory of how you got from no code to the
this point in time. Each commit s= hould add a feature and associated
documentation. It looks like most of = patches 0-6 can be one commit, same for
some subsequent patches.

= Heres an example of some issues I've noticed:

In patch 2 you add tes= ts/test=5Fssh.robot but then in patch 7 you delete
tests/test=5Fssh.rob= ot. There are several examples of a patch adding a
significant chunk of = work and then a later patch removing this chunk of work, if
after the se= ries the chunk of work won't be in the repo then it shouldn't have
appea= red in the series at all.

Patch 26 "Fix errors" should be = squashed into the broken commit. Patch 22 looks
that way too and Patch 1= 7. The reason we do this is that way when something
does go wrong and so= mething doesn't work it makes it much easier to find the
broken patch, y= ou should never submit a patches that is broken even if it is
fixed dire= ctly after this breaks the assumption that every commit we make is
good = and therefore when chasing a bug if a broken patch is found, is it likelyto be that bug, much easier to automate.

Your patch 28 that "= Adds one more ssl test" should be squashed into the patch
adding th= e ssl tests, add them all once for this series. You can always more
work= on more later but this series you have one patch adding ssl tests, haveall the ssl tests you're adding in that patch.

Hopefully that makes= sense, please ask if I've just rambled!

Cyril

> = https= ://github.com/openbmc/openbmc-test-automation/pull/1
> <= br>> Chris Austen (16):
>   Added DIMM specific inventory tes= ts
>   base add for sensor tests
>   fixed review com= ments
>   created loops for tests
>   Add initial res= t test cases     Support to test Null properties
>   A= dded full sensor suite
>   Update README.md
>   Handl= e new rest client from Brad
>   Added DIMM specific inventory te= sts
>   base add for sensor tests
>   fixed review co= mments
>   created loops for tests
>   Add initial re= st test cases     Support to test Null properties
>   = Added full sensor suite
>   Update README.md
>   Hand= le new rest client from Brad
>
> Manjunath A Kumatagi (34):>   Added git ignore file
>   Use argumnet file with py= robot and added instructions to READEME file.
>   Update README = for better html display
>   Added Inventory list testcase
>= ;   fixed errors in tox and generate=5Fargumentfile.sh script
> =   Moved Read Attribute and Read Properties to rest=5Fclient
> &n= bsp; fix review comment
>   Added SSL tests
>   Ran t= he tests and fixed errors in SSL tests
>   Modified the doc
&= gt;   Added firmware version validation test
>   update rea= dme file with tox version
>   Fix errors
>   Fix Erro= rs in test=5Fobmcrest.robot and removed the empty testcases.
>  = Added one more testcase to test non-ssl connection to port 443
> &nb= sp; OpenBMC REST Testing
>   updated the REST API for firmware v= ersion
>   Added git ignore file
>   Use argumnet fil= e with pyrobot and added instructions to READEME file.
>   Updat= e README for better html display
>   Added Inventory list testca= se
>   fixed errors in tox and generate=5Fargumentfile.sh script=
>   Moved Read Attribute and Read Properties to rest=5Fclient>   fix review comment
>   Added SSL tests
> &nb= sp; Ran the tests and fixed errors in SSL tests
>   Modified the= doc
>   Added firmware version validation test
>   u= pdate readme file with tox version
>   Fix errors
>  = Fix Errors in test=5Fobmcrest.robot and removed the empty testcases.
&g= t;   Added one more testcase to test non-ssl connection to port 443>   OpenBMC REST Testing
>   updated the REST API for f= irmware version
>
> Manjunath Kumatagi (2):
>   fir= st commit
>   first commit
>
> manjunath (6):
&g= t;   added files
>   added log statement
>   add= ed testcase for etc passwd
>   added files
>   added = log statement
>   added testcase for etc passwd
>
>= mkumatag (2):
>   Update README.md
>   Update README= .md
>
>
> =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F= =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F= =5F=5F=5F=5F=5F=5F=5F
> openbmc mailing list
> openbmc@lists.oz= labs.org
> https://lists.ozlabs.org/listinfo/openbmc

=5F=5F= =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F= =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F
openbmc mai= ling list
openbmc@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc<= /tt>



--1__=EABBF5ABDFA7BACE8f9e8a93df938690918cEABBF5ABDFA7BACE-- --0__=EABBF5ABDFA7BACE8f9e8a93df938690918cEABBF5ABDFA7BACE Content-type: image/gif; name="graycol.gif" Content-Disposition: inline; filename="graycol.gif" Content-ID: <1__=EABBF5ABDFA7BACE8f9e8a93df938690918cEAB@> Content-Transfer-Encoding: base64 R0lGODlhEAAQAKECAMzMzAAAAP///wAAACH5BAEAAAIALAAAAAAQABAAAAIXlI+py+0PopwxUbpu ZRfKZ2zgSJbmSRYAIf4fT3B0aW1pemVkIGJ5IFVsZWFkIFNtYXJ0U2F2ZXIhAAA7 --0__=EABBF5ABDFA7BACE8f9e8a93df938690918cEABBF5ABDFA7BACE--