Re: [Freeipa-devel] Travis CI broke after merging PR 177
On 12/13/2016 09:41 AM, Martin Babinsky wrote: Hi list, https://github.com/freeipa/freeipa/pull/177 was recently merged despite causing nearly half of the tests in our Travis CI gating to fail. This broke Travis CI for all other PR that were rebased after this merge, causing false negative errors everywhere. Fraser reverted the offending commits in https://github.com/freeipa/freeipa/pull/329 which restored Travis to original state (never mind PEP8 errors they were in the original code already). Regarding this issues I have two questions: a) should I merge https://github.com/freeipa/freeipa/pull/329 and thus revert the breakage in order to unblock other contributors? Given the current traffic I think it is sufficient to wait for us to investigate and produce a fix. If not, please scream loudly. b) what can we improve to make the results of CI more visible to contributors? I think that I should sit down with Martin 2 and investigate the possibility to send notifications about negative CI results (sufficient IMO) to the mailing list. In the meanwhile I would like to ask all reviewers to carefully check the output of failed Travis CI runs. If the job fails, you will see the results at the very end of the log. There are two sections: PEP8 errors and test output. You can expand both of them to see what went wrong and report it to the PR author if necessary. The reviewer and author can then use the very same tool used in CI [1] to reproduce the failures locally. Using '--no-cleanup' option during the run [2] leaves behind a running container which you can attach to and investigate further. [1] https://github.com/freeipa/ipa-docker-test-runner [2] https://github.com/freeipa/ipa-docker-test-runner/blob/master/README.md If you have any additional questions/suggestions about Travis feel free to contact me. The PR fixing failing Travis CI wss merged to master now. If you rebase and sync your branches the checks should be green again (well unless you broke something in your PR but you can not blame the regression for that anymore ;)). -- Martin^3 Babinsky -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] Travis CI broke after merging PR 177
On 12/13/2016 01:41 PM, Fraser Tweedale wrote: On Tue, Dec 13, 2016 at 01:11:37PM +0100, Martin Babinsky wrote: On 12/13/2016 01:07 PM, Fraser Tweedale wrote: On Tue, Dec 13, 2016 at 09:41:40AM +0100, Martin Babinsky wrote: Hi list, https://github.com/freeipa/freeipa/pull/177 was recently merged despite causing nearly half of the tests in our Travis CI gating to fail. This broke Travis CI for all other PR that were rebased after this merge, causing false negative errors everywhere. Fraser reverted the offending commits in https://github.com/freeipa/freeipa/pull/329 which restored Travis to original state (never mind PEP8 errors they were in the original code already). Regarding this issues I have two questions: a) should I merge https://github.com/freeipa/freeipa/pull/329 and thus revert the breakage in order to unblock other contributors? Given the current traffic I think it is sufficient to wait for us to investigate and produce a fix. If not, please scream loudly. Definitely not, I am using this PR as a playground to poke the CI in various ways. Also, proper fix would be best :) See also my other mail (I would have replied here but fetchmail was not fetching mail and I missed it until just now >_<). b) what can we improve to make the results of CI more visible to contributors? I think that I should sit down with Martin 2 and investigate the possibility to send notifications about negative CI results (sufficient IMO) to the mailing list. Or the commit author. It would be *great* if the test job, in event of failure, would collect all the obvious logs and dump them somewhere as artifacts. That would be great but AFAIK Travis CI does support only archiving artifacts into a running AWS instance[1] which we do not have and would have to buy, run and maintain ourselves. For now we have to inspect the log dumps generated in the job output. Do the containers not have internet access? That is all you really need, surely. The log dumps are not enough... `tail -n 5000' is not enough :) Even if there is no other way besides S3, it might still be worth it. Travis imposes hard 10k line limit in the job logs that's why I had to truncate them, we have discussed this issue ad nauseum. And AFAIK the workers are not accessible from outside. In the meanwhile I would like to ask all reviewers to carefully check the output of failed Travis CI runs. If the job fails, you will see the results at the very end of the log. There are two sections: PEP8 errors and test output. You can expand both of them to see what went wrong and report it to the PR author if necessary. The reviewer and author can then use the very same tool used in CI [1] to reproduce the failures locally. Using '--no-cleanup' option during the run [2] leaves behind a running container which you can attach to and investigate further. [1] https://github.com/freeipa/ipa-docker-test-runner [2] https://github.com/freeipa/ipa-docker-test-runner/blob/master/README.md If you have any additional questions/suggestions about Travis feel free to contact me. -- Martin^3 Babinsky -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code [1] https://docs.travis-ci.com/user/uploading-artifacts/ -- Martin^3 Babinsky -- Martin^3 Babinsky -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] Travis CI broke after merging PR 177
On Tue, Dec 13, 2016 at 01:11:37PM +0100, Martin Babinsky wrote: > On 12/13/2016 01:07 PM, Fraser Tweedale wrote: > > On Tue, Dec 13, 2016 at 09:41:40AM +0100, Martin Babinsky wrote: > > > Hi list, > > > > > > https://github.com/freeipa/freeipa/pull/177 was recently merged despite > > > causing nearly half of the tests in our Travis CI gating to fail. This > > > broke > > > Travis CI for all other PR that were rebased after this merge, causing > > > false > > > negative errors everywhere. > > > > > > Fraser reverted the offending commits in > > > https://github.com/freeipa/freeipa/pull/329 which restored Travis to > > > original state (never mind PEP8 errors they were in the original code > > > already). > > > > > > Regarding this issues I have two questions: > > > > > > a) > > > > > > should I merge https://github.com/freeipa/freeipa/pull/329 and thus revert > > > the breakage in order to unblock other contributors? Given the current > > > traffic I think it is sufficient to wait for us to investigate and > > > produce a > > > fix. If not, please scream loudly. > > > > > Definitely not, I am using this PR as a playground to poke the CI in > > various ways. Also, proper fix would be best :) See also my other > > mail (I would have replied here but fetchmail was not fetching mail > > and I missed it until just now >_<). > > > > > b) > > > > > > what can we improve to make the results of CI more visible to > > > contributors? > > > I think that I should sit down with Martin 2 and investigate the > > > possibility > > > to send notifications about negative CI results (sufficient IMO) to the > > > mailing list. > > > > > Or the commit author. > > > > It would be *great* if the test job, in event of failure, would > > collect all the obvious logs and dump them somewhere as artifacts. > > > > That would be great but AFAIK Travis CI does support only archiving > artifacts into a running AWS instance[1] which we do not have and would have > to buy, run and maintain ourselves. For now we have to inspect the log dumps > generated in the job output. > Do the containers not have internet access? That is all you really need, surely. The log dumps are not enough... `tail -n 5000' is not enough :) Even if there is no other way besides S3, it might still be worth it. > > > In the meanwhile I would like to ask all reviewers to carefully check the > > > output of failed Travis CI runs. If the job fails, you will see the > > > results > > > at the very end of the log. There are two sections: PEP8 errors and test > > > output. You can expand both of them to see what went wrong and report it > > > to > > > the PR author if necessary. > > > > > > The reviewer and author can then use the very same tool used in CI [1] to > > > reproduce the failures locally. Using '--no-cleanup' option during the > > > run > > > [2] leaves behind a running container which you can attach to and > > > investigate further. > > > > > > [1] https://github.com/freeipa/ipa-docker-test-runner > > > [2] > > > https://github.com/freeipa/ipa-docker-test-runner/blob/master/README.md > > > > > > If you have any additional questions/suggestions about Travis feel free to > > > contact me. > > > > > > -- > > > Martin^3 Babinsky > > > > > > -- > > > Manage your subscription for the Freeipa-devel mailing list: > > > https://www.redhat.com/mailman/listinfo/freeipa-devel > > > Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code > > [1] https://docs.travis-ci.com/user/uploading-artifacts/ > > -- > Martin^3 Babinsky -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] Travis CI broke after merging PR 177
On 12/13/2016 01:07 PM, Fraser Tweedale wrote: On Tue, Dec 13, 2016 at 09:41:40AM +0100, Martin Babinsky wrote: Hi list, https://github.com/freeipa/freeipa/pull/177 was recently merged despite causing nearly half of the tests in our Travis CI gating to fail. This broke Travis CI for all other PR that were rebased after this merge, causing false negative errors everywhere. Fraser reverted the offending commits in https://github.com/freeipa/freeipa/pull/329 which restored Travis to original state (never mind PEP8 errors they were in the original code already). Regarding this issues I have two questions: a) should I merge https://github.com/freeipa/freeipa/pull/329 and thus revert the breakage in order to unblock other contributors? Given the current traffic I think it is sufficient to wait for us to investigate and produce a fix. If not, please scream loudly. Definitely not, I am using this PR as a playground to poke the CI in various ways. Also, proper fix would be best :) See also my other mail (I would have replied here but fetchmail was not fetching mail and I missed it until just now >_<). b) what can we improve to make the results of CI more visible to contributors? I think that I should sit down with Martin 2 and investigate the possibility to send notifications about negative CI results (sufficient IMO) to the mailing list. Or the commit author. It would be *great* if the test job, in event of failure, would collect all the obvious logs and dump them somewhere as artifacts. That would be great but AFAIK Travis CI does support only archiving artifacts into a running AWS instance[1] which we do not have and would have to buy, run and maintain ourselves. For now we have to inspect the log dumps generated in the job output. In the meanwhile I would like to ask all reviewers to carefully check the output of failed Travis CI runs. If the job fails, you will see the results at the very end of the log. There are two sections: PEP8 errors and test output. You can expand both of them to see what went wrong and report it to the PR author if necessary. The reviewer and author can then use the very same tool used in CI [1] to reproduce the failures locally. Using '--no-cleanup' option during the run [2] leaves behind a running container which you can attach to and investigate further. [1] https://github.com/freeipa/ipa-docker-test-runner [2] https://github.com/freeipa/ipa-docker-test-runner/blob/master/README.md If you have any additional questions/suggestions about Travis feel free to contact me. -- Martin^3 Babinsky -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code [1] https://docs.travis-ci.com/user/uploading-artifacts/ -- Martin^3 Babinsky -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] Travis CI broke after merging PR 177
On Tue, Dec 13, 2016 at 09:41:40AM +0100, Martin Babinsky wrote: > Hi list, > > https://github.com/freeipa/freeipa/pull/177 was recently merged despite > causing nearly half of the tests in our Travis CI gating to fail. This broke > Travis CI for all other PR that were rebased after this merge, causing false > negative errors everywhere. > > Fraser reverted the offending commits in > https://github.com/freeipa/freeipa/pull/329 which restored Travis to > original state (never mind PEP8 errors they were in the original code > already). > > Regarding this issues I have two questions: > > a) > > should I merge https://github.com/freeipa/freeipa/pull/329 and thus revert > the breakage in order to unblock other contributors? Given the current > traffic I think it is sufficient to wait for us to investigate and produce a > fix. If not, please scream loudly. > Definitely not, I am using this PR as a playground to poke the CI in various ways. Also, proper fix would be best :) See also my other mail (I would have replied here but fetchmail was not fetching mail and I missed it until just now >_<). > b) > > what can we improve to make the results of CI more visible to contributors? > I think that I should sit down with Martin 2 and investigate the possibility > to send notifications about negative CI results (sufficient IMO) to the > mailing list. > Or the commit author. It would be *great* if the test job, in event of failure, would collect all the obvious logs and dump them somewhere as artifacts. > In the meanwhile I would like to ask all reviewers to carefully check the > output of failed Travis CI runs. If the job fails, you will see the results > at the very end of the log. There are two sections: PEP8 errors and test > output. You can expand both of them to see what went wrong and report it to > the PR author if necessary. > > The reviewer and author can then use the very same tool used in CI [1] to > reproduce the failures locally. Using '--no-cleanup' option during the run > [2] leaves behind a running container which you can attach to and > investigate further. > > [1] https://github.com/freeipa/ipa-docker-test-runner > [2] https://github.com/freeipa/ipa-docker-test-runner/blob/master/README.md > > If you have any additional questions/suggestions about Travis feel free to > contact me. > > -- > Martin^3 Babinsky > > -- > Manage your subscription for the Freeipa-devel mailing list: > https://www.redhat.com/mailman/listinfo/freeipa-devel > Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
