Darker GH action checks against HEAD commit -> no diff

[MatthijsBurgh]

The Darker GitHub action is not working. As without specifying the revision argument, it checks against HEAD.

So in GH actions, it will always check against the current HEAD, which means the diff is always empty. Of course I could set up some custom logic to get the revision for a push or pull request in my workflow. But it would be really helpful for everybody if would be included in the action.

So the cases to consider in my opinion are just the push and PR events for now:
Push event: Get the diff compared to last run (on this branch)
PR event: Get the diff for the entire PR.
For my own CI, I have used the following logic, maybe you could use it: https://github.com/tue-robotics/tue-env/blob/master/ci/commit-range/action.yml (In case COMMIT_RANGE is empty, I go back to HEAD^. This should only cause issues when HEAD is the first commit in the repo.)

[akaihola]

Thanks @MatthijsBurgh for the report! I will return to this issue in the near future.

[jedie]

I used a work-a-round:

https://github.com/boxine/django-huey-monitor/blob/3a2b7a34f105bd5389338d6b1cf844fd486c1223/.github/workflows/pythonapp.yml#L27-L29

Then "master" is there and can be used for compare ;)

[akaihola]

This may be related to what @mayk0gan, @foxwhite25 and @jenshnielsen reported in comments for #264. Let's make sure those get fixed as well.

[MatthijsBurgh]

I have done some test runs. I do get the same error now as #264. Both 1.4.0 and master keep failing. Because the default darker version is still set to 1.3.2 in the action file. So that should be update.

So it is unclear to me at the moment if this related to #264 or that #264 is hiding my issue.

[MatthijsBurgh]

Ok, by manually forcing darker 1.4.0 is does run. But I think we still require some more git logic. As it keeps the revision argument still defaults to HEAD. And because there are no uncommitted changes in an action run. The diff will always be zero.

So it is needed to do some git revision logic in the action. (Or include this in the library. But I would go for the first option.)

As in my PR in my repo, I made changes that are not black(https://github.com/MatthijsBurgh/epydoc/pull/2/files#diff-26a8638e359a431fd5f8b5079abea3c6c6eaa4fc203fc95d8aa651ee66d88934R92) But the CI doesn't fail. (https://github.com/MatthijsBurgh/epydoc/runs/5138102045?check_suite_focus=true#step:3:7)

[jenshnielsen]

@MatthijsBurgh The simplest is to use the -r flag with the branch you want to compare against. The script that I suggested in #264 does that by hardcoding the master branch. It would be better if the default was something like -r {targetbranch}

[MatthijsBurgh]

@jenshnielsen Yes I know I can manually override the revision. But that would only solve the issue for me. I think it much better to include this logic in the action. So that everybody benefits from it. As for now the action is not really working out of the box.

See my first comment (#260 (comment)) for the logic I use in my CI setup. Depending on the desired behaviour we could just copy that or need to modify it.

[akaihola]

In a feature branch centered workflow, I've found -r master... to always be the most sensible setting. But now that there are lots of repositories where there's a main branch instead of master, we can't make a safe assumption for the value of that setting. Any ideas?

[jenshnielsen]

For pull request based runs it would seem to me that the environmental variable GITHUB_BASE_REF is the right thing to use. Documented here https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables

[akaihola]

I'd appreciate reviews of #282, @MatthijsBurgh @jedie @jenshnielsen. Would you accept an invitation as a collaborator on the Darker repository so I can assign you as reviewers?

[akaihola]

GITHUB_BASE_REF is the right thing to use

So @jenshnielsen you're saying --check --diff --revision=${GITHUB_BASE_REF} would be a sensible default options for Darker in the GH Action?

[akaihola]

Since GITHUB_BASE_REF exists only when the GitHub Action is triggered by a pull_request or pull_request_target event, we need to define --revision= differently for action runs in the main branch. Would --revision=HEAD^ make sense there so we would be checking modified lines in the last commit of the main branch?

[jenshnielsen]

Yes That seems good to me but I have not tested it yet. That is the pull request target. I have not thought a lot about the branch build

[akaihola]

To be honest, I didn't quite understand what the context is exactly with the pull_request_target trigger. Does HEAD still point to the branch, or to a speculative merged state of the main branch?

[MatthijsBurgh]

I'd appreciate reviews of #282, @MatthijsBurgh. Would you accept an invitation as a collaborator on the Darker repository so I can assign you as reviewers?

Yes, that is fine. For reviewing and other small tasks, I am in.

[jenshnielsen]

@akaihola Happy to be a collaborator. I left a non collaborator review now

[MatthijsBurgh]

My opinion on what revision you should check:
For a PR run you want to check all commits on the feature branch compared to the target branch. IMO you shouldn't include any commits that are on the target branch, but not on the feature branch (so when the feature branch is behind the target branch).
For a push run, you want to include all commits since the last CI run. That could be just the last commit if you always push one commit at a time. But it could be a range of commits, when you only push after a few commits.

This should also be the logic from my CI(https://github.com/tue-robotics/tue-env/blob/master/ci/commit-range/action.yml). The logic resolving a empty commit range to HEAD^ is somewhere else. So that should still be included.

[MatthijsBurgh]

#282 didn't fix the issue.

[akaihola]

@MatthijsBurgh I finally took a look at your tue-env/ci/commit-range/action.yml, and it looks really good.

I couldn't find a license in that repository, so I can't just copy that code into a PR here. If you create that PR instead, I would think you'll then take the responsibility that you have the right to have your code distributed here under the 3-clause BSD license.

[MatthijsBurgh]

I have created a PR to add a License to the tue-env repo. I think once that is merged, we can create a PR and just mention the source in the commit/PR message. @akaihola you agree?

[akaihola]

Agree!

[akaihola]

tue-robotics/tue-env#604 seems to be merged now, so we can proceed!