-
Notifications
You must be signed in to change notification settings - Fork 847
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
chore: Remove not used cache in InstanceCredentialProvider #5888
Conversation
Signed-off-by: Xuanwo <github@xuanwo.io>
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.
Thank you @Xuanwo
@@ -424,9 +424,6 @@ fn canonicalize_headers(header_map: &HeaderMap) -> (String, String) { | |||
/// <https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/configuring-instance-metadata-service.html> | |||
#[derive(Debug)] | |||
pub struct InstanceCredentialProvider { | |||
// https://github.com/apache/arrow-rs/issues/5884 | |||
#[allow(dead_code)] |
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.
🤔 sine this is a pub field, this might qualify as a breaking change -- in which case we may need to hold the PR until main is open for breaking changes again. I plan to file a ticket to discuss the plan in the next day or two (I keep meaing to do it but I haven't found the time yet)
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.
This struct is crate private so this is not a breaking change
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.
gah! it would be nice if we followed the pattern to mark all such structs pub(crate)
so it is clearer from the definition
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.
Probably a good first issue, FWIW very little is actually public
Which issue does this PR close?
Closes #5884.
Rationale for this change
I have confirmed that this cache is not used.
What changes are included in this PR?
Remove
cache
inInstanceCredentialProvider
Are there any user-facing changes?
None