autest isn’t required for merges anymore, since the results of the tests are inconsistent.
We have been using the CI system for awhile now to smoke test the PRs. The builds, regressions, clang-analyzer, and clang-format have been stable and rarely fail. If there is a problem with the CI in the future we should make sure it gets fixed. There have been commits that didn’t pass the basic testing and were not reviewed before getting committed to master. The result has been breaking the builds and introducing code into ATS that should have had changes. In the worst case you can sill push directly to git bypassing the entire process. This isn’t recommended in the general case. -Bryan > On May 27, 2017, at 3:25 PM, James Peach <jpe...@apache.org> wrote: > > > > On 16 May 2017, at 19:42, Phil Sorber wrote: > >> One of the things that was accomplished at the ATS Summits over the past >> several days (in addition to clearing up many many Coverity issues) was >> getting the build system integration with GitHub fixed up. We now have real >> commit status updates from Jenkins so all those ugly comments are gone. We >> were actually getting rate limited by GitHub and it brought our build >> system to a crawl. We've required all these builds status to be passing >> before the PR can be merge. So no more accidental merges or getting >> confused by all the comments from Jenkins. >> >> Which brings me to the reason for this email. We are also trying out >> requiring reviews. So now you have to have one positive review for a PR to >> be mergeable. > > I have 3 concerns: > > - The tests are not stable enough that we don’t get spurious failures > - Knowledge of the CI system is not widespread enough that everyone can fix > it when it goes wrong > - It is common to need to fix up PRs when committing them and this process > prevents that AFAICT > > While I appreciate the CI and I think it is a great improvement we should not > be preventing committers committing to master. > > J
