-
Notifications
You must be signed in to change notification settings - Fork 104
cleanup: refactor headers in credentials #1741
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
cleanup: refactor headers in credentials #1741
Conversation
coryan
left a 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.
Thanks for the PR! I started most builds and we left some comments. @harkamaljot or @sai-sunder-s may have further comments.
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. 🚀 New features to boost your workflow:
|
|
@sai-sunder-s: Here's the updated PR. Lmk if this addresses your feedback/comment. cc/ @coryan |
|
Thank you for the change! |
|
/gcbrun |
This PR is an effort to resolve issue #1686.
While working on this PR, I considered two approaches:
headersimplementation to theCredentialsTraittrait that has the common logic that can then be shared across the credentialsOption 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
utilsmodule, following the structure recommended in the Rust Book here. Currently we have only theheaders_utilutility function, but more utility functions could be added when/if needed.