5 ms·

> deferring all testing and linting to the CI process is an anti-pattern I'm confused as to why this is an anti-pattern? My understanding is that the CI pipeli

> deferring all testing and linting to the CI process is an anti-pattern I'm confused as to why this is an anti-pattern? My understanding is that the CI pipeline should run unit tests and linting for every commit. But at the same time, developers should run their tests before pushing code.

charcircuit 4y ago
Linters and tests should pass before code is rebased onto master. YOLOing code onto master is an antipattern because if your code breaks the build then you are holding up everyone else from being able to make changes until yours gets reverted. If linters and tests are already passing there is a very good chance the rebase won't break anything.
- viraptor 4y ago
  You can go a step further. Instead of merging anything, you can tell the ci to merge it. Then ci can make a "merge-test" branch, run all the tests on it, and if they pass, then ff-merge it to master for real. No need for "good chance" or rebasing just to keep up with master. It does take some extra work though, because GH and others don't really support this out of the box.
  - marcusarmstrong 4y ago
    Gitlab does support this with “Merged Result pipelines”[0]. We use them extensively alongside their merge train functionality to sequential is everything and it’s fantastic. [0]: https://docs.gitlab.com/ee/ci/pipelines/merged_results_pipelines.html https://docs.gitlab.com/ee/ci/pipelines/merged_results_pipel...
  - zelos 4y ago
    Isn’t this the way everyone works? Write code, run some tests/linting locally, then create a PR to master and the CI server runs all the tests and reports pass/fail. Changes without a pass can’t be merged to master.
    - charcircuit 4y ago
      The extra step is that once the change is accepted the process of rebasing it or merging it into master is done by a bot that checks if it would break master before preceding to do it. My issue with this approach is that it becomes tricky to scale since you can only have one job running at a time. Allowing master to potentially break scales better because you can run a job for each commit on master which hasn't been evaluated. Technically you could make that approach work by instead of rebasing onto master rebasing onto the last commit that is being tested, but this adds extra complexity which I don't think standard tooling can easily handle.
      - viraptor 4y ago
        > you can only have one job running at a time https://zuul-ci.org/ https://zuul-ci.org/ and some other systems solve it by optimistic merges. If there's already a merge job running, the next one assumes that will succeed. And tests on merged master + first change + itself. If anything breaks, the optimistic merges are dropped from the queue and everything starts from the second chance only. Openstack uses it and it works pretty well if the merges typically don't fail.
    - throwaway858 4y ago
      This can result in a broken master if there were new commits added to master since the pull request was submitted (but before it is merged). The solution is to require that all PR must be rebased/synced to master before they can be merged. GitHub has an option for enforcing this. The downside is that this often results in lots of re-running of tests.
cntainer 4y ago
Emphasis on all :). Of course the CI should always run them, but that should normally be as a confirmation/safeguard. I've seen too many cases where the devs wouldn't even run the code locally. They would push it and expect the CI to do all the work. That's how you get shitty CI that is always broken.
- deleted 4y ago
  [deleted]
- thiht 4y ago
  That’s why you use branches though. You can break the CI on your own branch as much as you want, it’s nobody’s business. But a broken CI on a dev branch MUST prevent merging to a release branch. If you allow devs to push directly on release branch, thus breaking the CI, you’re absolutely doing it wrong.
  - cassianoleal 4y ago
    The TBD [0] crowd disagrees with you. I agree with you though. Not that I don't see the value proposed by TBD, but I think you can have >90% of said value and none of the downsides using a well thought out branching strategy. [0] Trunk-Based Development: https://trunkbaseddevelopment.com/ https://trunkbaseddevelopment.com/
    - smallnix 4y ago
      From the linked Website: > Depending on the team size, and the rate of commits, short-lived feature branches are used for code-review and build checking (CI). [...] Very small teams may commit direct to the trunk.
      - cassianoleal 4y ago
        Indeed. That seems to be a new-ish addition. I'm glad they now admit alternate approaches.
    - 4y ago

charcircuit 4y ago

Linters and tests should pass before code is rebased onto master. YOLOing code onto master is an antipattern because if your code breaks the build then you are holding up everyone else from being able to make changes until yours gets reverted. If linters and tests are already passing there is a very good chance the rebase won't break anything.

viraptor 4y ago
You can go a step further. Instead of merging anything, you can tell the ci to merge it. Then ci can make a "merge-test" branch, run all the tests on it, and if they pass, then ff-merge it to master for real. No need for "good chance" or rebasing just to keep up with master. It does take some extra work though, because GH and others don't really support this out of the box.
- marcusarmstrong 4y ago
  Gitlab does support this with “Merged Result pipelines”[0]. We use them extensively alongside their merge train functionality to sequential is everything and it’s fantastic. [0]: https://docs.gitlab.com/ee/ci/pipelines/merged_results_pipelines.html https://docs.gitlab.com/ee/ci/pipelines/merged_results_pipel...
- zelos 4y ago
  Isn’t this the way everyone works? Write code, run some tests/linting locally, then create a PR to master and the CI server runs all the tests and reports pass/fail. Changes without a pass can’t be merged to master.
  - charcircuit 4y ago
    The extra step is that once the change is accepted the process of rebasing it or merging it into master is done by a bot that checks if it would break master before preceding to do it. My issue with this approach is that it becomes tricky to scale since you can only have one job running at a time. Allowing master to potentially break scales better because you can run a job for each commit on master which hasn't been evaluated. Technically you could make that approach work by instead of rebasing onto master rebasing onto the last commit that is being tested, but this adds extra complexity which I don't think standard tooling can easily handle.
    - viraptor 4y ago
      > you can only have one job running at a time https://zuul-ci.org/ https://zuul-ci.org/ and some other systems solve it by optimistic merges. If there's already a merge job running, the next one assumes that will succeed. And tests on merged master + first change + itself. If anything breaks, the optimistic merges are dropped from the queue and everything starts from the second chance only. Openstack uses it and it works pretty well if the merges typically don't fail.
  - throwaway858 4y ago
    This can result in a broken master if there were new commits added to master since the pull request was submitted (but before it is merged). The solution is to require that all PR must be rebased/synced to master before they can be merged. GitHub has an option for enforcing this. The downside is that this often results in lots of re-running of tests.

cntainer 4y ago

Emphasis on all :). Of course the CI should always run them, but that should normally be as a confirmation/safeguard. I've seen too many cases where the devs wouldn't even run the code locally. They would push it and expect the CI to do all the work. That's how you get shitty CI that is always broken.

deleted 4y ago
[deleted]
thiht 4y ago
That’s why you use branches though. You can break the CI on your own branch as much as you want, it’s nobody’s business. But a broken CI on a dev branch MUST prevent merging to a release branch. If you allow devs to push directly on release branch, thus breaking the CI, you’re absolutely doing it wrong.
- cassianoleal 4y ago
  The TBD [0] crowd disagrees with you. I agree with you though. Not that I don't see the value proposed by TBD, but I think you can have >90% of said value and none of the downsides using a well thought out branching strategy. [0] Trunk-Based Development: https://trunkbaseddevelopment.com/ https://trunkbaseddevelopment.com/
  - smallnix 4y ago
    From the linked Website: > Depending on the team size, and the rate of commits, short-lived feature branches are used for code-review and build checking (CI). [...] Very small teams may commit direct to the trunk.
    - cassianoleal 4y ago
      Indeed. That seems to be a new-ish addition. I'm glad they now admit alternate approaches.
  - 4y ago