scientific-python / circleci-artifacts-redirector-action

GitHub Action to add a GitHub status link to a CircleCI artifact.
MIT License
14 stars 8 forks source link

Action produces hundreds of failed builds #27

Open asmeurer opened 2 years ago

asmeurer commented 2 years ago

If you look at https://github.com/sympy/sympy/actions/workflows/docs-preview.yml, you can see there are a ton of failed builds for the docs preview action. I think this is because it runs every time there is a status change from any CI job, but it only makes sense when the status change actually comes from the CircleCI job.

Is it possible to make the action not run at all in these cases? It's obviously not a huge issue, but the current way is fairly confusing, and makes the "all workflows" list harder to read https://github.com/sympy/sympy/actions.

By the way, do you know why half of those jobs are indicated as being started by me, even for commits that I had nothing to do with? How does GitHub determine who the author of a "status" update is?

larsoner commented 2 years ago

Is it possible to make the action not run at all in these cases?

Not sure, someone will have to look at the GitHub actions docs to see if the on: status can be made more selective in YAML, or if it has to short-circuit in the action itself.

If we can't make the YAML more selective (which is what I'd expect), we should at least make it exit gracefully rather than emit an error.

How does GitHub determine who the author of a "status" update is?

No idea...

asmeurer commented 2 years ago

It seems like if could be used to do this, if I am reading https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#status correctly.

However, I tried it at https://github.com/sympy/sympy/pull/23801 and I can't get it to work. Any suggestions? It's super annoying to test this since it has to be merged into master before it actually does anything, so I'd prefer to minimize the number of iterations here. Or if you have a better way to test the action out that would be helpful.

larsoner commented 2 years ago

It's super annoying to test this since it has to be merged into master before it actually does anything, so I'd prefer to minimize the number of iterations here.

I would test it out by iterating by cycling through two steps

  1. Committing directly to the master branch of your fork of this repository some changeset you think should work
  2. Pushing an empty commit to a dummy branch in your fork of this repository, with a PR open to merge dummy into master

Every dummy commit you push in step (2) should immediatly allow you to see if step (1) had the effect you wanted.

Once it does, you can open a PR to merge your master branch into the upstream one here (with or without cleaning up the commits first -- I'll just squash + merge so it won't make much difference)

rgommers commented 1 year ago

I was looking into this as well. I think on: [status] is the wrong trigger, and it cannot be made more specific later on. The right one seems to be repository_dispatch.

See also https://stackoverflow.com/questions/67330220/trigger-github-action-after-circleci-workflow-or-job.

larsoner commented 1 year ago

I think on: [status] is the wrong trigger,

In some sense yes, because it has to look at all status events to find the right one. In another sense no, in that it does work without needing to make any changes to your CircleCI config...

The right one seems to be repository_dispatch.

... unlike this solution. I think this would require anyone who wanted to use this solution to add such a dispatch to their CircleCI job.

So I think we could advertise two ways to do it:

  1. The current ugly way that leads to a lot of extra logging and runs, but works.
  2. A better way that uses repo dispatch

@rgommers I think you might be able to try the repo dispatch approach (2) on SciPy already by modifying the CircleCI job to dispatch the event, and modify the circleci-artifacts-redirector-action YAML to accept it, right?

larsoner commented 1 year ago

I might give this a shot in this repo. It'll be easier and way less noisy than trying it in SciPy :)

larsoner commented 1 year ago

I think this might not be tractable. To launch a GitHub repo event, whatever emits/posts the repository_dispatch event must have write access to the repo:

https://docs.github.com/en/rest/repos/repos#create-a-repository-dispatch-event

You can do this via:

  1. A GitHub app with the correct permissions, but CircleCI is not an app, or
  2. A personal access token with repo-level permissions

For (2) to work, a maintainer would need to create a personal access token with access to the repo, put this in the CircleCI config, and make this visible/accessible/usable by forked PRs, which is unsafe because anybody could in the CircleCI job echo or post this token somewhere else and thereby gain repo-level permissions. I think that's the case anyway...

rgommers commented 1 year ago

Argh, that's not nice. Agreed that this sounds too risky. Thanks for looking into it!

asmeurer commented 1 year ago

It still might be possible to make this work with if. I couldn't figure it out, but the docs certainly seem to imply that it ought to work.

larsoner commented 1 year ago

Maybe with https://github.com/larsoner/circleci-artifacts-redirector-action/pull/32 ?

asmeurer commented 1 year ago

Maybe we should open a feedback issue about this at https://github.com/community/community/discussions/categories/general-feedback

bsipocz commented 1 year ago

Now that we start using access tokens anyway, I wonder whether this could be fixed, too? https://github.com/larsoner/circleci-artifacts-redirector-action/issues/27#issuecomment-1243760872

larsoner commented 1 year ago

I am not optimistic because of this part of that comment:

and make this [token] visible/accessible/usable by forked PRs, which is unsafe because anybody could in the CircleCI job echo or post this token somewhere else and thereby gain repo-level permissions. I think that's the case anyway...

bsipocz commented 1 year ago

Ahh, I was way too enthusiastic than a careful reader then.

bsipocz commented 1 year ago

To follow-up: I could skip running the workflow for non-circleCI related jobs with a conditional in jobs:, but they are still listed as cancelled ones in the actions list (I couldn't make a conditional work in the on: section).

I also tried to use check_run, but couldn't manage to trigger the workflow with that one, and even if it worked I believe it would not be the right approach either as it totally disentangles from the circleCI status and trigger only once all CI checks have finished/succeded.