Automatically update PRs with dist changes

foolip commented 1 month ago

Running npm run dist after changes on a PR is tedious. Let's have a GitHub Actions workflow which updates them and pushes a new commit.

ddbeck commented 3 weeks ago

There's been several PRs toward satisfying this, though it's not working perfectly just yet:

I suspect once all of these have merged, we'll have a better sense of whether this is done done.

ddbeck commented 1 week ago

OK, I regret spending as much time as I have on this. GitHub Actions: here be dragons. I'll try to summarize the situation here: without a dedicated bot user or using someone's personal access token (thus making it appear as if that person is the author of the commit), this can only be done in a partial, not all that useful way. 💢

Here's the longer version:

Automatically committing requires elevated permissions, but accepting contributions from forks means the risk of arbitrary code execution from untrusted sources. To deal with this safely, you need to refresh dists in separate steps:

In a reduced permissions environment (i.e., in a pull_request event workflow), run untrusted code to generate an artifact with the things we care about (e.g., a patch that contains the results of npm run dist).
In an elevated permissions environment, get the patch artifact, apply it, and commit it to the PR branch. The commit is what requires permissions (even though we're not committing the repository itself—one could). Don't execute the results. This is safe!
In a reduced permissions environment (in a pull_request event workflow), run the tests with the new commit applied.

This is a little indirect, but it's manageable. The trouble is that step 2, using the GitHub Actions token, cannot re-trigger the pull_request event (example). This is to prevent endlessly recursive workflows.

The only way to override this specific limitation is to use another user to do the commit.

ddbeck commented 1 week ago

Mulling this over a bit, alternatives open to us:

Create and maintain a "bot user" (a GitHub account specifically for this purpose) or use a real-person's PAT (attributing all such commits to that person—weird).
Refresh dists during tests, instead of on-demand. For example, always produce a patch and, if the test workflow says it would pass—either before or after the patch—declare that test workflow successful. Then, in a separate workflow with elevated permissions, apply successful patches to the branch PR. Strictly speaking, the commit at the head of the branch would be untested (but would satisfy the GitHub branch protection rules).
Like the previous item, but with elevated permissions, so that commit and test could happen in the same workflow, guaranteeing that the test ran on the given commit. You'd have to limit this somehow, by only running unsafe jobs on branches on the web-platform-dx/web-features repo or PRs from known contributors' forks. Unfriendly to new or infrequent contributors.
Like the previous item, but instead allowlisting on the branch's home repo, strictly check the list of files changed. If anything other than features/**/*.yml file changed, then that PR would be ineligible for an automatic refresh. I think that would be OK—parsing untrusted YAML is probably low risk, right? 🤨
Generate a comment that says what to do/what changes would result, but don't automatically commit. In effect, give up. This would make the PRs easier to review, but eventually somebody has to run npm run dist and push a commit themselves.

captainbrosset commented 4 hours ago

I have to say that automatically creating or updating files in a PR branch, on GitHub, from a different user, seems a bit odd to me.

Dist files are generated files, yes, but on our repo, they're very special because we want to carefully review them (unlike most generated files that are usually just boring build artifacts). They're generated only because we use a bunch of scripts to help authors write the browser support data. But, in the end, authors are still the ones who decide what the support data is, by including/excluding BCD keys. In my mind, generating dist files by using a script is more of an author convenience, almost akin to a code formatter.

Have we considered generating dist files via local tooling, such as a build config in VS Code, or a pre-commit hook (or both)? There could also be a github workflow that fails if the dist file is missing or incorrect, and warns the author to go and update it.

I contributed a few features to the repo and have not really been bothered by the fact that I had to re-generate dist files. The only thing that bothers me a little is having to enter the name of the YAML file in npm run dist features/file-name.yml. I wish I could just do npm foo and that would check my new file, generate whatever dist files need to be generated, etc.

web-platform-dx / web-features

Automatically update PRs with dist changes #1158