Status
DONE
Stakeholderscore development team
OutcomeSee proposal below
Due date
Owner

Background

Many members of the development team report frustration with the workflow experience related to integration testing. The key pain point is broken code being merged to dev prior to the automated run of integration tests. In short: unit tests pass and allow the merge, then integration tests fail after broken code is in dev. 



Current systemProposed new system
Order of testing in CI related to PR activity
  • Open PR cues automated unit tests in CI
  • Passed unit tests allow merge
  • Merge of PR cues automated integration tests in CI

Pro

Con
  • RISK if integration tests fail, the broken code is already in dev

Discussion

Please add your thoughts, experiences, and recommendations to this section. Select edit to add content.

Your thoughts and experiences

Your nameYour thoughts and experiences
Kelly FosterI heard Adam and Tomáš mention a security risk of running both unit and integration tests at the opening of a PR. I don't understand the risk or the significance of taking on this risk.
Łukasz GołębiewskiWe need to make sure that integration tests are run prior to merging to dev. This process could be semi-automatic and we would just need to make sure that the last (and not every) commit of a PR is tested. Once a PR has been reviewed and the reviewer has verified that no one is trying to exploit the vulnerabilities of our CI infrastructure, they could start the integration tests by some simple means, e.g. by commenting on github or by adding a label to the issue. A green integration tests report for a PR would be a prerequisite for merging
Artur GajowyI understand the risk, but don't deem it significant or worth mitigating, even without additional precautions. I believe having the i-tests run only for the members of https://github.com/rchain-drone/ would be more than enough.
Artur GajowyWhy do we need the privileged mode for docker containers anyway? To package the current code into a docker container? How I see it, the container should be done in a way that allows us to "upgrade" it with newer code by simply mounting a volume with the updated code. We don't have to build or upload any docker artifacts in the PR build. Doesn't that invalidate the 'docker.sock' issue?
Artur Gajowy

Running i-tests on every push to a PR seems wasteful (e.g. I do many pushes per PR, often within minutes). So maybe it should be done on demand. The 'demand' should also automatically include 'ok, I want to merge this'.

Artur GajowyBecause all of the above being solved/worked around by bors, the semantic conflict issue we haven't touched upon yet, and because of how bors optimizes the number of tests runs used to merge a set of PRs I recommend going with bors.
Tomáš Virtushttps://www.projectatomic.io/blog/2015/08/why-we-dont-let-non-root-users-run-docker-in-centos-fedora-or-rhel/

Recommendations for changes

Your nameChange recommendation+/- (feedback from others)
Kelly FosterI'd like to see both unit and integration tests run via ci before a PR merge. Reasons we didn't do this in the past related to ability of Travis alone and later Travis + GitLab to support this. I think Drone can support this now. 
AdamIntroduce https://github.com/bors-ng/bors-ng or https://github.com/palantir/bulldozer and let them manage testing and merging instead of relying on GitHub.I've reviewed both of their docs, and only bors mentions a sufficient feature set. And is much better documented.
Artur Gajowy

Try setting up bors using their public instance (should be all we ever need). Just follow "setting up bors".

Have the build for "staging" and "trying" branches include i-tests.

Don't limit push access to bors only until we're happy with the integration.

Document that we'll be using "bors delegate+" instead of "bors r+" most of the time. See reference.

Include all developers as reviewers in bors settings.

Make a POC on a fork/different repo first and consider if bors can push to non-master branches (we need dev).


Tomáš Virtushttps://github.com/opencontainers/runc#rootless-containers
https://github.com/genuinetools/img/







Action items

Proposal

  • Implement bors to support the following process:
    • PR opened (run compilation and unit tests)
      • Run tests at every push to PR
    • PR review
    • Run of all tests (unit+integration) at approval of PR (via comment on GitHub)
    • Merge blocked until tests pass (we think merge is automated by bors)