make CIPD url customizable using FLUTTER_STORAGE_BASE_URL #94137
Conversation
|
cc @chenglu |
chenglu
left a comment
There was a problem hiding this comment.
LGTM, thanks for doing this! 👍👍
| String get cipdBaseUrl { | ||
| final String? overrideUrl = _platform.environment['FLUTTER_STORAGE_BASE_URL']; | ||
| if (overrideUrl == null) { | ||
| return 'https://chrome-infra-packages.appspot.com/dl'; |
There was a problem hiding this comment.
We already defined a hidden CIPD base URL here:
Should we hard-code this again?
There was a problem hiding this comment.
I am removing this constant in this PR so that people do not accidentally use it in the future. Of course, the bar to make this mistake is still not very high (anyone can just create a new constant), but at least making it a local constant doesn't make it easily accessible. Actually, now that I think of it, I should probably leave a comment warning people about this.
There was a problem hiding this comment.
So it seems we should have a policy about this in the future? URLs can be easily introduced, but the mistake can be avoided most time with guidance.
There was a problem hiding this comment.
I'm not sure how to make a policy such that it's effective. A policy is just a document somewhere that someone has to read, preferably before they start making changes to the code. Maybe such document exists already, and I didn't read it. I would be more likely to read a comment in the code, although that's still not bullet proof.
I'll delegate to @christopherfujino, who is the tech lead for the tool, to decide if a policy can help here.
There was a problem hiding this comment.
Agreed with a lower priority.
There was a problem hiding this comment.
I'll update a doc about it, but agreed, the "policy" can only be enforced in code review. We did not have a policy banning this before, because we had not considered the effect on users in China. AFAIK, the only previous usage of this CIPD hack was for Fuchsia-related assets, which I'm guessing did not have many/any Chinese users.
…94490) * make CIPD url customizable using FLUTTER_STORAGE_BASE_URL (#94137) * 'Update Engine revision to 06a7363 for beta release 2.8.0-3.3.pre' * Update plugin lint test for federated url_launcher plugin (#94374) Co-authored-by: Yegor <yjbanov@google.com> Co-authored-by: Jenn Magder <magder@google.com>
…lutter#94490) * make CIPD url customizable using FLUTTER_STORAGE_BASE_URL (flutter#94137) * 'Update Engine revision to 06a7363 for beta release 2.8.0-3.3.pre' * Update plugin lint test for federated url_launcher plugin (flutter#94374) Co-authored-by: Yegor <yjbanov@google.com> Co-authored-by: Jenn Magder <magder@google.com>
add Flutter Web cipd Support
Allow customizing the base CIPD URL via
FLUTTER_STORAGE_BASE_URLas follows:FLUTTER_STORAGE_BASE_URLis not specified, use https://chrome-infra-packages.appspot.com/dl as the base CIPD URL.FLUTTER_STORAGE_BASE_URL/flutter_infra_release/cipdThis way maintainers of Flutter SDK artifact mirrors can include some of the CIPD packages that are needed to build for some platforms. For example, this can be used to mirror the build of CanvasKit.
Fixes #92357