Skip to content

Conversation

@chrischiedo
Copy link
Contributor

This PR is an effort to resolve issue #1686.

While working on this PR, I considered two approaches:

  1. Creating a utility function that takes the necessary parameters and returns the required headers
  2. Adding a default headers implementation to the CredentialsTrait trait that has the common logic that can then be shared across the credentials

Option 1 was more straight forward (and easier to implement). Option 2, on the other hand, is a bit more involving (requiring more changes to existing code). Therefore, for this PR, I went with Option 1.

Note: I have added a utils module, following the structure recommended in the Rust Book here. Currently we have only the headers_util utility function, but more utility functions could be added when/if needed.

Copy link
Collaborator

@coryan coryan left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I started most builds and we left some comments. @harkamaljot or @sai-sunder-s may have further comments.

@codecov
Copy link

codecov bot commented Apr 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.79%. Comparing base (4079324) to head (4358516).
Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1741      +/-   ##
==========================================
- Coverage   95.83%   95.79%   -0.04%     
==========================================
  Files          49       50       +1     
  Lines        1919     1904      -15     
==========================================
- Hits         1839     1824      -15     
  Misses         80       80              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chrischiedo
Copy link
Contributor Author

@sai-sunder-s: Here's the updated PR. Lmk if this addresses your feedback/comment.

cc/ @coryan

@sai-sunder-s
Copy link
Contributor

Thank you for the change!

@coryan
Copy link
Collaborator

coryan commented Apr 9, 2025

/gcbrun

@coryan coryan merged commit 092a574 into googleapis:main Apr 9, 2025
20 of 22 checks passed
@chrischiedo chrischiedo deleted the cleanup-refactor-headers-credentials branch April 13, 2025 06:39
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.

3 participants