Check Chromium revision pinning scripts into repo#4220
Conversation
jcscottiii
left a comment
jcscottiii
left a comment
There was a problem hiding this comment.
Thanks for adding these scripts. I added some comments to expand on how to deploy.
Also, you will find a comment on pinning our GitHub API version.
Other comments are nits or questions.
Let me know if you have any questions!
| The purpose of this script is to check the WPT CI check suite to see if all | ||
| tests passed for the new revision, and to update the pinned revision if so. | ||
|
|
||
| The current PR used for running the check suites is at https://github.com/web-platform-tests/wpt/pull/50375 |
There was a problem hiding this comment.
Could you add the command(s) to deploy these scripts in each section?
Also, do you know how to generate the GIT_CHECK_PR_STATUS_TOKEN stored in secret manager that is used in both of these scripts?
Also, do you know about the instructions for process_test_history?
There was a problem hiding this comment.
Any instructions on how to create these PRs in the future?
There was a problem hiding this comment.
Could you add the command(s) to deploy these scripts in each section?
I've been doing it through the GCP UI, but I'll look up and add instructions for this.
Also, do you know how to generate the GIT_CHECK_PR_STATUS_TOKEN stored in secret manager that is used in both of these scripts?
These are instructions I need to add as well, but I was thinking this might be better suited to live in our rotation docs.
Also, do you know about the instructions for process_test_history?
The instructions file build_test_history.py is what describes this, but I now realize I've not coordinated the names correctly here. It should probably be that "build_test_history" should be renamed to "process_test_history" to match the Cloud Function name.
There was a problem hiding this comment.
Any instructions on how to create these PRs in the future?
Do you mean creating new cloud functions in the future? Or making changes to these files?
There was a problem hiding this comment.
I've been doing it through the GCP UI, but I'll look up and add instructions for this.
That's good to know. I thought you were deploying it with the CLI. So I was curious what arguments you passed in. You could just reference this document and call out any gotchas that need to be filled in.
These are instructions I need to add as well, but I was thinking this might be better suited to live in our rotation docs.
That works!
The instructions file build_test_history.py is what describes this, but I now realize I've not coordinated the names correctly here. It should probably be that "build_test_history" should be renamed to "process_test_history" to match the Cloud Function name.
I saw that name and I was wondering if they were the same. Yeah could you either rename the file here or change the name in GCP? edit I see that you did that already.
Do you mean creating new cloud functions in the future? Or making changes to these files?
I meant this PR web-platform-tests/wpt#50375. But I guess let me ask these questions first:
- How often do you change this PR?
- Have we had other PRs like this in the past?
- How did you know what changes to make in the first place for this PR?
| # Reopen the PR to run the CI tests. | ||
| s = requests.Session() | ||
| s.headers.update({"Authorization": f"token {get_token()}"}) | ||
| url = f"https://api.github.com/repos/{REPO_OWNER}/{REPO_NAME}/pulls/{PR_NUMBER}" |
There was a problem hiding this comment.
We should either:
- Use the GitHub client python library
- Manually pin the
X-GitHub-Api-Versionheader in each API call.
| s.headers.update({ | ||
| "Authorization": f"token {get_token()}" | ||
| }) | ||
| url = f"https://api.github.com/repos/{REPO_OWNER}/{REPO_NAME}/commits/{get_sha()}/check-suites" |
There was a problem hiding this comment.
We should either:
- Use the GitHub client python library
- Manually pin the
X-GitHub-Api-Versionheader in each API call.
Right now, we are expecting the response to be a certain format. If the API version changes and the shape of the response changes, we will start to get KeyError. (for example when we go to check the conclusion.
There was a problem hiding this comment.
Good idea. I think I'll approach using #2 and make changes once I confirm everything functions as expected.
| PR_NUMBER = "50375" | ||
| REPO_OWNER = "web-platform-tests" | ||
| REPO_NAME = "wpt" |
There was a problem hiding this comment.
Thoughts on making these environment variables? A user shouldn't really need to change the code just to update these variables. Instead, they could quickly deploy the app with updated variables. You could do more of these variables but this set would be a good start.
[1]
There was a problem hiding this comment.
I can definitely make these environment variable instead 🙂
Co-authored-by: James C Scott III <jcscottiii@users.noreply.github.com>
|
Thanks for taking such a good look at this James! 🙂 |
|
|
||
| def get_sha(repo_owner: str, repo_name: str, pr_number: str) -> str: | ||
| """Get head sha from PR.""" | ||
| url = f'https://api.github.com/repos/{repo_owner}/{repo_name}/pulls/{pr_number}' |
3 participants
This adds the source code for the GCP cloud functions that are run to automatically update the pinned Chromium revision in the WPT repository.
Subsequent changes will come with deployment and update instructions.