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

Reland "Find manifest for download by tags instead of commits" #14209

Merged
merged 1 commit into from
Dec 10, 2018

Conversation

foolip
Copy link
Member

@foolip foolip commented Nov 23, 2018

Reverted in #14208
as possible cause of #14207.

This reverts commit 5438f72.

Reverted in #14208
as possible cause of #14207.

This reverts commit 5438f72.
@foolip
Copy link
Member Author

foolip commented Nov 23, 2018

Putting up this PR so that I don't forget it later. Should wait until it's clear if the revert fixed the problem or not before relanding.

@@ -33,46 +33,53 @@ def should_download(manifest_path, rebuild_time=timedelta(days=5)):
return False


def git_commits(repo_root):
def merge_pr_tags(repo_root, max_count=50):
Copy link
Member Author

Choose a reason for hiding this comment

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

This can't be relanded as-is even if it wasn't the cause of the failures, because tags aren't fetched in the Taskcluster setup: #14207 (comment)

@foolip
Copy link
Member Author

foolip commented Nov 23, 2018

@jgraham @lukebjerring the original PR is a case where a full Taskcluster run that mimics as closely as possible the master-triggered runs would have been very valuable. What are the steps required to make that happen? Seems like something we should capture in WPT 2019 priorities.

@lukebjerring
Copy link
Contributor

lukebjerring commented Nov 23, 2018

Basically, needs to run on a fork with Taskcluster enabled (always instead of current master-only). I'll do that for you.
I'll also whitelist you for the wpt.fyi checks, which won't be full runs, but still helpful.

@lukebjerring lukebjerring mentioned this pull request Nov 23, 2018
@foolip
Copy link
Member Author

foolip commented Nov 23, 2018

If we wanted a mechanism to request a full run from a PR in this repo, what are the constraints? I guess adding/removing labels wouldn't work, only updating branches would?

If Taskcluster had deep integration with the new checks API, then an action (button) that says "request full run" would be neat, but I doubt it'd be high on the Taskcluster team's priorities.

@lukebjerring
Copy link
Contributor

I'm not familiar with TaskCluster's capabilities, but if we can branch based on a PR's labels, that would be nice and clean - we add "if the PR is labelled run-me" to the set of conditions.
If not, and until TaskCluster supports ad-hoc running, we could consider having some kind of "full-taskcluster-runs/wpt" repo fork and creating an equivalent PR with the hack, using a checks action.

@lukebjerring
Copy link
Contributor

You can see the results of the full runs here:
https://github.com/web-platform-tests/wpt/pull/14212/checks?check_run_id=34456218

@foolip
Copy link
Member Author

foolip commented Nov 23, 2018

Thanks! Clearly relanding this would break things again, so definitely I shouldn't do that yet :)

@gsnedders
Copy link
Member

@foolip we have #13263 filed for that

@jgraham
Copy link
Contributor

jgraham commented Dec 10, 2018

We now fetch tags in taskcluster since #14315 landed, so I think this can land.

@foolip
Copy link
Member Author

foolip commented Dec 10, 2018

Yay! @jgraham, feel free to approve and merge, at a time when you can keep on eye on master to see if it broke everything again.

@jgraham jgraham merged commit 94ac45d into master Dec 10, 2018
@gsnedders gsnedders deleted the foolip/reland-14025 branch December 11, 2018 14:11
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.

5 participants