5

Right now we have Codacy monitoring a DEV branch, and as per recommended practices, whenever we do something, we create a personal branch of DEV, work on that, and then merge back in. Thing is, if Codacy finds a problem, we have to branch out of DEV, revise, then merge back in again. Meanwhile, DEV has this defective code, so we have to undo that merge, etc, etc. Lots of room for error if you're panicking because the guys overseas are coming online soon!

Three acceptable solutions come to mind, might be more:

  • configure Codacy to review all branches of a monitored branch post-commit
  • we tend to name our branches consistently, so could specify regular expressions
  • configure GitHub and/or Codacy to prevent pulls of a monitored branch if there are pending Codacy issues

Are any of these possible?

  • 2
    You should be restricting any pushes to `DEV` directly and take them only through pull request which should only be allowed if the codacy checks have passed. – Tarun Lalwani Jul 08 '19 at 02:22
  • @TarunLalwani Is there a way to configure GitHub and/or Codacy to prevent pulls if there are pending Codacy issues? –  Jul 08 '19 at 10:14
  • Yes, that is what we use in our org, I will post the settings details – Tarun Lalwani Jul 08 '19 at 10:48

2 Answers2

3

Under /settings/branches you can define "Branch protection rules" for DEV and make Codacy status as required. You will not be able to merge the PR until you handle your issues.

machadoit
  • 136
  • 4
  • But will developers still be able to pull from DEV if there are Codacy issues? –  Jul 08 '19 at 10:36
  • If you follow the @TarunLalwani suggestion, and do PRs to DEV, you can have DEV without issues, and you don't have to worry about anyone pulling DEV on a bad state – machadoit Jul 08 '19 at 10:39
1

As discussed follow the below approach

You should be restricting any pushes to DEV by making it a protected branch. No direct commits will be allowed and only through pull request commits can be merged. In the same, you can mandate the codacy checks to be passed for a merge to be allowed

See below settings for sample

Github settings

Update 12th Jul:

Lots of points clarified on comments, so adding those explanations to answer

Developer 1 -> Commits to DEV_1 Branch -> Raise a Pull Request #1 to Merge DEV_1 to DEV Developer 2 -> Commits to DEV_2 Branch -> Raise a Pull Request #2 to Merge DEV_2 to DEV

The PR #1 and PR#2 cannot be merged as we have specified that the codacy status check has to Pass.

This status check from Codacy, will do all the testing that you want to do. Once the Codacy test have passed it will update the PR and merging will get enabled based on the status of the Codacy results

The Codacy test is nothing but a Post commit check. Github doesn't allow pre-commit hooks. Gitlab does allow you to have pre-commit hooks but its best to use post-commit hooks on a PR when working with Github

Post hooks

Tarun Lalwani
  • 142,312
  • 9
  • 204
  • 265
  • Codacy won't scan until post-commit, so doesn't the bad code needs to be present first in DEV? Regarding "**Requires branches to be up to date before merging**", so this implies each branch need to be manually enabled for Codacy? Or will it do it automatically? We want to reduce the amount of "dicking around" with Codacy, i.e. set it and forget it. –  Jul 08 '19 at 12:31
  • `Require branches to be up to date before meeting`, means if two pull request have been created to merge with your branch, then after one of them is approved, the other once will be retested with latest code – Tarun Lalwani Jul 08 '19 at 14:22
  • For pre commit scan you need to use prehooks which are local to your machine, so tools like codacy will only work on post commit. That's why you want DEV to be protected branch and rest all to merge to DEV – Tarun Lalwani Jul 08 '19 at 14:25
  • Sorry, I'm not understanding what you are writing. Using this setting, do the working branches need to be manually enabled for Codacy? –  Jul 08 '19 at 15:20
  • 1
    @Buddy, You need to enable `DEV` branch for analysis in Codacy. So, when a PR is raised to be merged against `DEV` the test will be executed. You don't need to enable each branch that developers will create in Codacy, you only need to enable the branch to which they want to merge – Tarun Lalwani Jul 09 '19 at 05:16
  • The `DEV` branch has been enabled. But it's still letting the bad code get merged before doing the checks. I think I'm tripping up on: *When enabled, commits must first be pushed to another branch, then merged or pushed directly to a branch that matches this rule after status checks have passed.* It looks like we have to create a monitored `pre-DEV`, it has a post-commit hook for Codacy to scan, then if it passes, merge into `DEV`? That complicates things though, developers pull from `DEV` but commit to `pre-DEV`, and an extra step to merge into `DEV`? –  Jul 10 '19 at 14:23
  • Also, I'm not seeing any prehook functionality for Codacy, is it maybe a feature of some other code analyzer? –  Jul 10 '19 at 14:29
  • In progress, I have to mask a lot of information for security reasons. Meanwhile, I like Yimin's approach to let Codacy review after pull requests. Checking that out, too. –  Jul 12 '19 at 12:34
  • @Buddy, my post was for that only? That you add the codacy status check on the pull request and don't allow the merge to happen until the test has passed and that is why you need to check "Require status checks to pass before merging". I assumed you already have the pull request status check setup, which is nothing else but a post commit-hook – Tarun Lalwani Jul 12 '19 at 12:51