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

Set User Agent Header for S3 Requests #2137

Merged
merged 6 commits into from
Mar 25, 2024
Merged

Conversation

ajschmidt8
Copy link
Contributor

Closes #2136.

This PR configures the S3 backend to use a custom HTTP client that has a user agent header set.

The user agent header enables sccache users to write AWS policies to accept or reject sccache requests to an S3 bucket based on their sccache version.

The user agent header value is like sccache/0.7.7.

Closes mozilla#2136.

This PR configures the S3 backend to use a custom HTTP client
which has a user agent header set.

The user agent header enables `sccache` users to write AWS policies to
accept or reject `sccache` requests to an S3 bucket based on their `sccache`
version.
@ajschmidt8
Copy link
Contributor Author

hmm. i'll have to fix this import issue.

src/cache/s3.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 30.49%. Comparing base (0cc0c62) to head (d9251b6).
Report is 16 commits behind head on main.

❗ Current head d9251b6 differs from pull request most recent head 96f7ed2. Consider uploading reports for the commit 96f7ed2 to get more accurate results

Files Patch % Lines
src/cache/s3.rs 33.33% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2137      +/-   ##
==========================================
- Coverage   30.91%   30.49%   -0.42%     
==========================================
  Files          53       53              
  Lines       20112    20486     +374     
  Branches     9755     9954     +199     
==========================================
+ Hits         6217     6248      +31     
- Misses       7922     8189     +267     
- Partials     5973     6049      +76     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

This PR looks good to me now!

@Xuanwo
Copy link
Collaborator

Xuanwo commented Mar 25, 2024

cc @sylvestre, would you like to take another look?

src/cache/s3.rs Outdated Show resolved Hide resolved
src/cache/s3.rs Outdated Show resolved Hide resolved
src/cache/s3.rs Outdated Show resolved Hide resolved
Co-authored-by: Sylvestre Ledru <sylvestre@debian.org>
@sylvestre sylvestre merged commit a859c3b into mozilla:main Mar 25, 2024
52 checks passed
@sylvestre
Copy link
Collaborator

thanks

@ajschmidt8 ajschmidt8 deleted the s3-user-agent branch March 25, 2024 17:22
@AJIOB
Copy link
Contributor

AJIOB commented Mar 26, 2024

@ajschmidt8 , just for note: do this really works in the prebuilt binary automatically (if you run the binary without cargo run)? Or we always need to define these Cargo variables externally?

@ajschmidt8
Copy link
Contributor Author

@ajschmidt8 , just for note: do this really works in the prebuilt binary automatically (if you run the binary without cargo run)? Or we always need to define these Cargo variables externally?

rust's env! macro captures environment variables at compile time: https://doc.rust-lang.org/std/macro.env.html.

and Cargo sets these values at compile time.

so they won't need to be defined externally. it will work in the prebuilt binary automatically.

@AJIOB, just curious. do you intend to use this feature? if so, what's your use case?

@AJIOB
Copy link
Contributor

AJIOB commented Mar 26, 2024

No, I just confused about backward compatibility with existing usages of S3 backend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set User Agent Header for S3 Requests
5 participants