Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Check Chromium revision pinning scripts into repo #4220
base: main
Are you sure you want to change the base?
Check Chromium revision pinning scripts into repo #4220
Changes from 1 commit
b58d6a3
c0fc2af
14d2975
6755747
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any instructions on how to create these PRs in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been doing it through the GCP UI, but I'll look up and add instructions for this.
These are instructions I need to add as well, but I was thinking this might be better suited to live in our rotation docs.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean creating new cloud functions in the future? Or making changes to these files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside the scope of this: But I wonder how this will work when we want Android given Chrome on Android runs on Chromium CI. We probably still want some coverage. Might be worth bringing this up to Weizhong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with the Chrome Android work, but is this shouldn't affect run for Chrome Android because it's considered a different product and does not use the pinned revision here (I might also be misunderstanding your comment 😅)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can definitely make these environment variable instead 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should either:
X-GitHub-Api-Version
header in each API call.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should either:
X-GitHub-Api-Version
header 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I think I'll approach using #2 and make changes once I confirm everything functions as expected.