Add S3 http2 toggle flag #604
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
nit: Should this be tested? I'm not sure how complex a test setup for this would be though. I'd be fine with just keeping it this way.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained
-- commits line 2 at r1:
nit: Maybe use something like "Add S3 http2 toggle flag" or similar so that this doesn't look like we're permanently disabling http2.
nativelink-config/src/stores.rs line 469 at r1 (raw file):
pub insecure_allow_http: bool, /// Disable http2 connections and rely on http1 only.
nit: Might make sense to elaborate the usecases for this.
There was a problem hiding this comment.
@aaronmondal I looked into this a bit before with another S3 client change, the challenge for unit tests is being able to dump the internal state of the client to know what configurations it holds. I did do something simple in a small example with the aws client by printing out the structure and it did serialize to something that contained structured information, what was clear is how to tease that information out in a testable way. Generally speaking testing client configuration sets aren't super useful, also testing them can catch silly configuration fails, so not completely against it. Testing this would generally be better in integration test settings or something where a fully binded client could be executed. I'll noodle a bit here, if doesn't seem fruitful will punt it for now.
Reviewable status: 1 of 1 LGTMs obtained
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit: Maybe use something like "Add S3 http2 toggle flag" or similar so that this doesn't look like we're permanently disabling http2.
Will do, need to get in the habit of assuming this is the commit message that lands and not an updated one from the github squash/merge dialog
nativelink-config/src/stores.rs line 469 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit: Might make sense to elaborate the usecases for this.
Will do, see how elaborate this can be expanded
adam-singer
force-pushed
the
adams/s3-disable-http2
branch
from
df9592a to
1afd21e
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), pre-commit-checks, publish-image, ubuntu-20.04, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable
Previously, adam-singer (Adam Singer) wrote…
Will do, need to get in the habit of assuming this is the commit message that lands and not an updated one from the github squash/merge dialog
Done.
nativelink-config/src/stores.rs line 469 at r1 (raw file):
Previously, adam-singer (Adam Singer) wrote…
Will do, see how elaborate this can be expanded
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04
There was a problem hiding this comment.
Reviewable status: 3 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04
adam-singer
force-pushed
the
adams/s3-disable-http2
branch
from
1afd21e to
0fbf896
Compare
Description
Add S3 TLS configuration to disable http2 in favor for http.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
not work as expected)
How Has This Been Tested?
Please also list any relevant details for your test configuration
Checklist
bazel test //...passes locallygit amendsee some docsThis change is