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

Don't use resources for draft PRs #46124

Closed
wants to merge 1 commit into from
Closed

Conversation

past
Copy link
Member

@past past commented May 6, 2024

This should fix #46122. I'm not sure how to test this apart from carefully monitoring the repo after merging. Ideas welcome!

Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stops us building the manifest for draft PRs? Is that what the Chromium exporter is looking at? But those manifests can (I think) be used in other CI jobs which we do want to run on draft PRs. So I'd expect some change on the Chromium side to fix this rather than on the wpt side.

@past
Copy link
Member Author

past commented May 7, 2024

Perhaps we should establish first whether the proposal in #46122 is in the right direction before discussing the specifics of this PR.

My impression is that draft PRs are usually (always?) in an incomplete state, where spending the necessary resources to run all the checks is not providing enough value. At least the draft PRs created by the Chromium exporter are almost certain to go through some further revisions before being ready to land in Chromium, never mind upstream. Are the workflows of other bots and users different that warrant the resource cost of running all the checks in draft state?

@jgraham
Copy link
Contributor

jgraham commented May 10, 2024

Oh, this is about export, not import. I think I misunderstood that.

I personally use draft PRs a lot specifically in order to run CI checks on complex or speculative changes before requesting review. So disabling CI entirely for draft PRs would break that workflow.

The gecko exporter doesn't create a PRs in draft mode (although it could), but it also doesn't create a PR at all until the change is on autoland, so the most common case is that no changes are made after the PR is created, and so it's reasonably efficient. Also, if the CI is run while the patches are on autoland it's possible that failures are noticed and fixed before the patches reach central (although I don't know how often that happens in practice).

The issue here seems to be that the Chromium exporter is creating PRs long before the patches are ready to land, and running CI on those is inefficient. I think the question here is what workflow you want on the Chromium side. If people are never looking at the CI results before the patch lands, could you defer creating the PRs? Or if they sometimes look, or look after a certain point in the lifecycle, what signal could we use to start running CI at that point?

We could, with some effort, build a system where draft PRs only run CI when they have a particular label (or don't run CI if they have a particular label, which the Chrome exporter could set by default).

@past
Copy link
Member Author

past commented May 20, 2024

As discussed offline we implemented a change in the Chromium exporter that should reduce resource consumption from PRs that are not yet ready. I'll close this.

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

Successfully merging this pull request may close these issues.

Do not run tests for PRs still in draft mode
4 participants