Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Remove the GitHub Action #1038

Closed
orta opened this issue May 8, 2020 · 8 comments
Closed

[RFC] Remove the GitHub Action #1038

orta opened this issue May 8, 2020 · 8 comments
Labels

Comments

@orta
Copy link
Member

orta commented May 8, 2020

Describe the bug

This repo opted into GitHub Actions as an "action" back during the alpha phase.

I don't think I thought enough about the repercussions, the CI for other takes much longer and is less predictable. I think it's just a much worse version of npx danger ci and we should probably stop making this repo show up as a GitHub Action.

This also means updating some docs, and I think there are recommendations in the errors which use the action syntax.

@orta orta added the bug label May 8, 2020
@orta orta assigned NotMoni and unassigned NotMoni May 8, 2020
@f-meloni
Copy link
Member

f-meloni commented May 8, 2020

Those are my 2c on this, but they come mainly from the experience I have with danger-swift/kotlin.
I see action as something is largely used with Danger (expecially for swift/kotlin, given there is no npx there), and I see a lot of potential for the future assuming you will be able to use GitHub packages with actions.
I suppose the cost of maintaining the support with actions is mainly related to use the json that actions provides, get the PR id, repo slug etc. more than actually maintaining the Dockerfile itself (that also can be used in other cases and I think we shouldn't remove anyway).
Then what would actual be the real action we should to for this issue?
Should we remove the Dockerfile complitely, or just avoid mentioning that you can use danger on actions with using?

@orta
Copy link
Member Author

orta commented May 8, 2020

I also think it's a bad abstraction in danger-swift too, you can't share the build phase at all and so the problem is exacerbated over there with danger taking many minutes to be used. I almost unilaterally recommend that people never use the actions IRL.

The biggest advantage is that it can have danger-js etc all set up ahead of time

I don't think it's about removing the docker support, that's tangential - people are free to set stuff up how they like. If they're using docker, then they are already used to slow builds. Today people think that the actions are the first thing you should use, and that's a worse experience then just doing Danger with the rest of your project.

@f-meloni
Copy link
Member

f-meloni commented May 8, 2020

I don't think it's about removing the docker support, that's tangential - people are free to set stuff up how they like. If they're using docker, then they are already used to slow builds.

Totally agree with this.

I also think it's a bad abstraction in danger-swift too, you can't share the build phase at all and so the problem is exacerbated over there with danger taking many minutes to be used.

This is indeed a problem, but has also some solutions.
One of them is create a Docker image on dockerhub, that takes 40 seconds to download and run Danger. I have an example here https://github.com/f-meloni/danger-swift-swiftlint-action
And i truly hope it will at one point support GitHub Packages too, that should be even faster to download (maybe even with a cache?) - We would need to public Danger as package in the repo for this obviously.

While danger-swift can build and run in about 2 min and 30 seconds in actions, in a good CI machine and an SPM env I saw it build and run a good number of checks in 15-20 seconds.
This is different for danger-kotlin that in the best case takes still around 2 mins at the moment (there is probably some improvement margin there though), and a solution like actions with packages - if it was working today - would probably be a good alternative.

Today people think that the actions are the first thing you should use, and that's a worse experience then just doing Danger with the rest of your project.

I totally agree with this, I think this partially comes with many of the restrictions actions currently has given is still not mature, but I think actions should be used with caution, but can be useful.

An example of real life usage:
I used danger to block a PR when a it was pointing a specific branch, but an approval from a specific user was missing.
This requires to be able to run Danger every time a review is submitted (we shouldn't need a new push to verify that the review was made).
Actions here works really well, and normal CI providers don't often offers a on_review trigger.
Then I made my own workflow with a Docker image that could run this checks on every review (but the first try I made was building the normal danger-swift action and was still acceptable), and the result was pretty good.

@mokagio
Copy link
Contributor

mokagio commented May 16, 2020

I think it's just a much worse version of npx danger ci

At Automattic, we started using the action from the marketplace, but quickly moved to running it directly because of how slower it was compared to Peril (which we are moving away from) and just running it directly.

@bakkerthehacker
Copy link

The current github action is broken: #557 (comment)

@fbartho
Copy link
Member

fbartho commented Jul 20, 2020

In our main application, we have our own Dockerfile for all of our Github Actions scripts (ruby+yarn+nodejs), so we just install DangerJS as a regular devDependency & then call yarn danger ci.


Could we instead document setting up a GitHub action that does npx danger ci? -- I worry that not being present in the marketplace is an issue for discoverability. Maybe we can have an action that points users to the more performant option?

@fbartho
Copy link
Member

fbartho commented Jan 25, 2022

Updating my comment from July 2020: since then, my teams have used DangerJS as a normal package.json devDependency, and it works great.

I have no objections to removing the Github Action -- except that some enterprising folks might feel inspired to publish their own unofficial ones, and muddy the waters.

@orta
Copy link
Member Author

orta commented Feb 1, 2022

I've hit the button to delist it, existing workflows will still "work" but folks won't find it in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants