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

Swap dirs-next dependency to cargo-team maintained home crate #1207

Merged
merged 2 commits into from
Jul 20, 2023

Conversation

utkarshgupta137
Copy link
Contributor

Motivation

dirs is an unnecessarily big dependency for a single function

Solution

Use the smaller, cargo-team maintained home crate

Signed-off-by: Utkarsh Gupta <utkarshgupta137@gmail.com>
@codecov
Copy link

codecov bot commented Apr 23, 2023

Codecov Report

Merging #1207 (6d9f590) into main (52a16ec) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1207      +/-   ##
==========================================
+ Coverage   73.16%   73.18%   +0.01%     
==========================================
  Files          75       75              
  Lines        6030     6030              
==========================================
+ Hits         4412     4413       +1     
+ Misses       1618     1617       -1     
Impacted Files Coverage Δ
kube-client/src/config/file_config.rs 77.52% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link
Member

@nightkr nightkr left a comment

Choose a reason for hiding this comment

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

The argument makes sense but this is also a pretty tiny impact one way or another.

@clux
Copy link
Member

clux commented Jul 20, 2023

I think this should be confirmed by someone on windows first. The home docs say this is changed.
This is what dirs-next is doing.

@clux
Copy link
Member

clux commented Jul 20, 2023

From some digging, the current* windows home folder goes to FOLDERID_Profile that is defaulted to
%USERPROFILE% (%SystemDrive%\Users\%USERNAME%) via knownfolderid docs, whereas home just looks up USERPROFILE. AFAIKT these are different ways to look-up the same thing?

@clux
Copy link
Member

clux commented Jul 20, 2023

So... client-go uses this implementation favouring HOME over USERPROFILE, the opposite of what the home crate does - but home crate has a reason for it. Our dirs-next uses FOLDERID_Profile which basically will favour USERPROFILE if its documentation is right.

IOW this doesn't change anything significant for us, and both answers are correct; it's client-go that is wrong 🙃

Going to merge this just because it's cargo maintained. This crate has already swapped once and there's only 2 people in the org that maintains the dirs-next. The long, historical thread on rust is probably one of the better sources of what's going on here with all these small lib crates: rust-lang/rust#71684

@clux clux added the changelog-change changelog change category for prs label Jul 20, 2023
@clux clux added this to the 0.85.0 milestone Jul 20, 2023
@clux clux changed the title Move to the smaller, cargo-team maintained home crate Swap dirs-next dependency to cargo-team maintained home crate Jul 20, 2023
@clux clux merged commit e6eba5d into kube-rs:main Jul 20, 2023
@nightkr
Copy link
Member

nightkr commented Jul 20, 2023

To bring some extra fun, the original dirs also seems to be resurrected these days...

@clux clux added the config Kube config related label Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-change changelog change category for prs config Kube config related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants