EIP Linting Bot

[alita-moore]

Github actions eip linting bot

In this process I converted the other python bot to typescript, added documentation, and I (mostly) untangled the originally nested logic.

next steps:

1. fix ERRORS as global, keep methods isolated, and make more robust against side-effect async errors
2. Build lots of tests (this repo needs them)

For now, though, the code achieves the goal of linting PRs and is able to be expanded much more easily

[lightclient]

I can review this more in-depth, but I don't think the actual code should live in this repo. I think you should create a repository to house this code and only install it in the CI here to use.

[alita-moore]

Yeah was thinking the same thing @lightclient but @MicahZoltu recommended letting it live here just for the sake of simplicity

[MicahZoltu]

I can review this more in-depth, but I don't think the actual code should live in this repo. I think you should create a repository to house this code and only install it in the CI here to use.

The way GitHub actions work, I actually think it ends up much simpler if we have the code live in this repository. Otherwise we would need a whole CI system for publishing the compiled/packaged GitHub action and then referencing it as a third party package.

[MicahZoltu]

.DSStore are OSX files I believe, they shouldn't be committed.

[alita-moore]

okay will remove

[MicahZoltu]

🤔 actually, to @lightclient's point, since this is TypeScript and GitHub actions only execute typescript, I think we still need a build process/CI to compile TS to JS? Is it possible to run TS directly in GitHub actions?

[alita-moore]

🤔 actually, to @lightclient's point, since this is TypeScript and GitHub actions only execute typescript, I think we still need a build process/CI to compile TS to JS? Is it possible to run TS directly in GitHub actions?

ncc takes care of this; dist/index.js is a pure js compile version that runs in actions

[MicahZoltu]

ncc takes care of this; dist/index.js is a pure js compile version that runs in actions

dist/index.js is derived from the .ts files I assume though, and that step should really be handled by a CI system and not manual by people making changes to the repository. That is, in general we should not be committing derived files.

I bet we can use ts-node to have the action actually execute as TypeScript (in-memory transpilation). Check out https://github.com/TypeStrong/ts-node. If you can pass CLI arguments to node then it should just be a matter of passing -r ts-node/register and then reference a .ts file as your entrypoint instead of dist/index.js. If you cannot pass CLI arguments to node as part of the action then we would need to create a 2-line index.js file (not in dist, just in the source root) that does something like require('ts-node').register({}) and then calls into a TS file (I'm not sure how to do that, ts-node docs should have more info on that).

[alita-moore]

@MicahZoltu Okay, I can do that. But another option could just be to do a simple action npm install && npm run build which would compile on action run time. Is there any particular benefit to executing typescript directly? As I understand it (and I could be wrong) a big benefit of ncc is that it avoids issues with packages.

example implementation of this:
alita-moore#2
https://github.com/alita-moore/EIPs/runs/2063313729

p.s. I implemented below as well

[MicahZoltu]

but if you don't set the secret it'll just fail every time and probably worth merging now

Sadly, if it hard fails it'll block CI. Fun fact, I actually cannot merge this PR because the bot is failing against this branch. 😬

[alita-moore]

@MicahZoltu I have defaulted the action to pass on error, so despite it failing it won't cause problems. the only behavior that will be prevented from failing behavior is the merging of a given PR. I haven't been able to implement a solid work around yet, but I'm certain that some exist. One example would be to just remove the secret whenever you want it to fail / be manual. Then when you want to run it, add a secret, click on the task on a given PR, and then re-run.

[MicahZoltu]

I believe only repository owners have the ability to change secrets, and the repository owners are not the editors. 😢

Does this help us at all? It sounds like exactly what we want, but maybe I'm missing something. https://docs.github.com/en/actions/managing-workflow-runs/manually-running-a-workflow

Also, I think we can make it so this particular GitHub action doesn't block merge on failure by adjusting the repository settings. I believe only @MadeofTin can do that though. Project page > Settings > Branches > Add Rule > Require status checks to pass before merging > deselect this github action.

[MicahZoltu]

Assuming @MadeofTin can make the above change, I think the best solution would be to disable the auto-merge feature, but enable errors. It will then auto-run on every PR and we can watch to see when it would have merged and when it would have not merged based on its success/failure status. Once we are comfortable it would be acting in the right times, we can then enable the auto-merge behavior.

Separately, assuming we can get manually run workflows setup, we could have a different workflow that does the same thing but does auto-merge which can be run manually. That way we can use it via manual trigger when the current bot is broken, which gives us another vector to test-in-production with it.

[alita-moore]

@MicahZoltu sounds good. I'll work on getting that all setup. Also is feasible to just have two workflows / secrets where one has the ability to merge and the other does not

Thanks for the link that should be easy to implement

[alita-moore]

@MicahZoltu

[alita-moore]

version 1.1.0 of the EIP bot has the ability to merge commented out

[alita-moore]

v1.1.1 also throws and error if anything except a merge occurs

[MicahZoltu]

Sorry for taking so long on reviewing this, been super busy lately.

@Souptacular I suspect we will need some changes to this repository when this is merged such as changing whether this status check is a required one or not:

Specifically, we would like to have this not block merging into master initially so we (editors) can monitor its behavior/output for a bit to make sure it all looks reasonable.

We may want to coordinate merging this to a time when all 3 of us are online so we can make sure we don't have downtime for the repository if it goes south. Alternatively, you can be replaced by anyone else with admin access to this repository.

[Souptacular]

Happy to coordinate. Best way to do this is to make a private group (maybe Discord?) with parties that are necessary and who will be monitoring. I can make you admin temporarily as well, but will try to be available as much as possible to help.