Princeton-LSI-ResearchComputing / tracebase

Mouse Metabolite Tracing Data Repository for the Rabinowitz Lab
MIT License
4 stars 1 forks source link

Superlinter is running duplicate workflow actions on github whenever we push to a PR #967

Closed hepcat72 closed 4 months ago

hepcat72 commented 4 months ago

BUG DESCRIPTION

Problem

I noted while working on some branch updating that I was frequently seeing duplicate superlinter runs. 1 of the runs seems unnecessary. I think that it is happening unintentionally due to changes to the superlinter yaml.

Steps to reproduce

  1. push a branch to github
  2. Create a PR
  3. [not sure if this is what's causing it] pull

Current behavior

I have seen this a few times now. One example was a push to an old PR (#901) after I did a rebase.

Expected behavior

When you push a commit to github (whether it's part of a PR or not), it should only trigger 1 workflow action.

Suggested Change

I think that the problem is this line in the new yaml:

https://github.com/Princeton-LSI-ResearchComputing/tracebase/blob/b56030d69a7939aabe14c12f6178606f44fc77b5/.github/workflows/superlinter.yml#L6

I think that when we push a change to a branch that has a pull request, the push action is initiating a workflow as usual, but the pull_request is also initiating a workflow action.

So I think we should remove line 6 (pull_request) from that yaml, as I think it's redundant. The push is triggering the workflow.

Comment

I can sort of understand the possible intent of adding pull_request to the yaml, but it seems a bit ambiguous. Intuitively, you think it would trigger upon creation of a pull request, but apparently it triggers whenever a branch that's a part of a PR changes. I think we should stick with just the push trigger. I use it before I create a PR, and if you ever don't want a push to trigger the workflow, you can just add [skip ci] to your commit message.


ISSUE OWNER SECTION

Assumptions

  1. List of assumptions made WRT the code
  2. E.g. We will assume input is correct (explaining why there is no validation)

Limitations

  1. A list of things this work will specifically not do
  2. E.g. This feature will only handle the most frequent use case X

Affected Components

Requirements

DESIGN

GUI Change description

None provided

Code Change Description

None provided

Tests

lparsons commented 4 months ago

I haven't looked into this too deeply, but there is a difference between running those triggers that is meaningful and I think we should be running both.

The push trigger would run the actions on the most recent commit pushed. The pull_request trigger would run the actions on the merge commit, which is not the same thing.

See https://medium.com/@lizjohnson_83228/the-difference-between-push-and-pull-request-triggers-in-github-d7c0f2bc19db.

hepcat72 commented 4 months ago

Huh. Perhaps this would prevent merging to main breaking main when there's a migrations conflict created by a separate PR that was merged in. If that's what it does, that's great. I'm not convinced it's efficient, but I'm convinced enough it may prevent problems. Closing.