<html><body><p>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.<br><br>Thanks,<br>Manjunath.<br><img width="16" height="16" src="cid:1__=EABBF5ABDFA7BACE8f9e8a93df938690918cEAB@" border="0" alt="Inactive hide details for Cyril Bur ---01/12/2016 05:41:04 AM---On Mon, 11 Jan 2016 10:49:26 -0600 OpenBMC Patches <openbmc-pat"><font color="#424282">Cyril Bur ---01/12/2016 05:41:04 AM---On Mon, 11 Jan 2016 10:49:26 -0600 OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:</font><br><br><font size="2" color="#5F5F5F">From: </font><font size="2">Cyril Bur <cyrilbur@gmail.com></font><br><font size="2" color="#5F5F5F">To: </font><font size="2">OpenBMC Patches <openbmc-patches@stwcx.xyz></font><br><font size="2" color="#5F5F5F">Cc: </font><font size="2">openbmc@lists.ozlabs.org</font><br><font size="2" color="#5F5F5F">Date: </font><font size="2">01/12/2016 05:41 AM</font><br><font size="2" color="#5F5F5F">Subject: </font><font size="2">Re: [PATCH openbmc-test-automation 00/60] Pull Manjunath's automation repository</font><br><font size="2" color="#5F5F5F">Sent by: </font><font size="2">"openbmc" <openbmc-bounces+mkumatag=in.ibm.com@lists.ozlabs.org></font><br><hr width="100%" size="2" align="left" noshade style="color:#8091A5; "><br><br><br><tt>On Mon, 11 Jan 2016 10:49:26 -0600<br>OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:<br><br>> Pull Manjunath's repository: </tt><tt><a href="https://github.com/mkumatag/openbmc-automation">https://github.com/mkumatag/openbmc-automation</a></tt><tt><br>> into openbmc.<br>> <br><br>Ummmm what happened here? It looks like the entire series is sent twice?<br><br>I've had an initial skim read and it looks to me like you can squash A LOT of<br>those patches together.<br><br>The commits don't need to be a story of how you got from no code to the<br>this point in time. Each commit should add a feature and associated<br>documentation. It looks like most of patches 0-6 can be one commit, same for<br>some subsequent patches.<br><br>Heres an example of some issues I've noticed:<br><br>In patch 2 you add tests/test_ssh.robot but then in patch 7 you delete <br>tests/test_ssh.robot. There are several examples of a patch adding a<br>significant chunk of work and then a later patch removing this chunk of work, if<br>after the series the chunk of work won't be in the repo then it shouldn't have<br>appeared in the series at all.<br><br>Patch 26 "Fix errors" should be squashed into the broken commit. Patch 22 looks<br>that way too and Patch 17. The reason we do this is that way when something<br>does go wrong and something doesn't work it makes it much easier to find the<br>broken patch, you should never submit a patches that is broken even if it is<br>fixed directly after this breaks the assumption that every commit we make is<br>good and therefore when chasing a bug if a broken patch is found, is it likely<br>to be that bug, much easier to automate.<br><br>Your patch 28 that "Adds one more ssl test" should be squashed into the patch<br>adding the ssl tests, add them all once for this series. You can always more<br>work on more later but this series you have one patch adding ssl tests, have<br>all the ssl tests you're adding in that patch.<br><br>Hopefully that makes sense, please ask if I've just rambled!<br><br>Cyril<br><br>> </tt><tt><a href="https://github.com/openbmc/openbmc-test-automation/pull/1">https://github.com/openbmc/openbmc-test-automation/pull/1</a></tt><tt><br>> <br>> Chris Austen (16):<br>> Added DIMM specific inventory tests<br>> base add for sensor tests<br>> fixed review comments<br>> created loops for tests<br>> Add initial rest test cases Support to test Null properties<br>> Added full sensor suite<br>> Update README.md<br>> Handle new rest client from Brad<br>> Added DIMM specific inventory tests<br>> base add for sensor tests<br>> fixed review comments<br>> created loops for tests<br>> Add initial rest test cases Support to test Null properties<br>> Added full sensor suite<br>> Update README.md<br>> Handle new rest client from Brad<br>> <br>> Manjunath A Kumatagi (34):<br>> Added git ignore file<br>> Use argumnet file with pyrobot and added instructions to READEME file.<br>> Update README for better html display<br>> Added Inventory list testcase<br>> fixed errors in tox and generate_argumentfile.sh script<br>> Moved Read Attribute and Read Properties to rest_client<br>> fix review comment<br>> Added SSL tests<br>> Ran the tests and fixed errors in SSL tests<br>> Modified the doc<br>> Added firmware version validation test<br>> update readme file with tox version<br>> Fix errors<br>> Fix Errors in test_obmcrest.robot and removed the empty testcases.<br>> Added one more testcase to test non-ssl connection to port 443<br>> OpenBMC REST Testing<br>> updated the REST API for firmware version<br>> Added git ignore file<br>> Use argumnet file with pyrobot and added instructions to READEME file.<br>> Update README for better html display<br>> Added Inventory list testcase<br>> fixed errors in tox and generate_argumentfile.sh script<br>> Moved Read Attribute and Read Properties to rest_client<br>> fix review comment<br>> Added SSL tests<br>> Ran the tests and fixed errors in SSL tests<br>> Modified the doc<br>> Added firmware version validation test<br>> update readme file with tox version<br>> Fix errors<br>> Fix Errors in test_obmcrest.robot and removed the empty testcases.<br>> Added one more testcase to test non-ssl connection to port 443<br>> OpenBMC REST Testing<br>> updated the REST API for firmware version<br>> <br>> Manjunath Kumatagi (2):<br>> first commit<br>> first commit<br>> <br>> manjunath (6):<br>> added files<br>> added log statement<br>> added testcase for etc passwd<br>> added files<br>> added log statement<br>> added testcase for etc passwd<br>> <br>> mkumatag (2):<br>> Update README.md<br>> Update README.md<br>> <br>> <br>> _______________________________________________<br>> openbmc mailing list<br>> openbmc@lists.ozlabs.org<br>> </tt><tt><a href="https://lists.ozlabs.org/listinfo/openbmc">https://lists.ozlabs.org/listinfo/openbmc</a></tt><tt><br><br>_______________________________________________<br>openbmc mailing list<br>openbmc@lists.ozlabs.org<br></tt><tt><a href="https://lists.ozlabs.org/listinfo/openbmc">https://lists.ozlabs.org/listinfo/openbmc</a></tt><tt><br></tt><br><br><BR>
</body></html>