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

Prefer HOME environment variable #1263

Merged
merged 4 commits into from
Apr 4, 2021

Conversation

twz123
Copy link
Contributor

@twz123 twz123 commented Sep 6, 2019

The Java system property is filled by a syscall to determine the user's home directory. This is not matching the typical implementation of the docker and kubectl binaries, which prefer the HOME OS environment variable over the syscall.

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Which is actually your use case in which both values are different ?

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

.

@twz123
Copy link
Contributor Author

twz123 commented Jan 28, 2020

I discovered this during the execution of a CI build, where the HOME variable was changed inside the build and the login to the Docker registry was made by docker login. The dmp wasn't picking up the login credentials in that case. My workaround was to also define JAVA_TOOL_OPTIONS="-Duser.home=.... Took me quite a while to figure out the problem. That's why I thought it might be a good idea to actually file a PR here.

@sonarcloud
Copy link

sonarcloud bot commented Jan 28, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

Merging #1263 (b77caf1) into master (fafa324) will increase coverage by 0.02%.
The diff coverage is 87.50%.

@@             Coverage Diff              @@
##             master    #1263      +/-   ##
============================================
+ Coverage     61.99%   62.02%   +0.02%     
- Complexity     2148     2150       +2     
============================================
  Files           166      166              
  Lines          9470     9472       +2     
  Branches       1438     1438              
============================================
+ Hits           5871     5875       +4     
+ Misses         3084     3083       -1     
+ Partials        515      514       -1     
Impacted Files Coverage Δ Complexity Δ
.../maven/docker/access/DockerConnectionDetector.java 62.00% <0.00%> (ø) 5.00 <0.00> (ø)
...a/io/fabric8/maven/docker/util/DockerFileUtil.java 82.30% <100.00%> (+1.26%) 37.00 <1.00> (ø)
...ain/java/io/fabric8/maven/docker/util/EnvUtil.java 79.41% <100.00%> (+0.51%) 67.00 <3.00> (+2.00)
...o/fabric8/maven/docker/util/VolumeBindingUtil.java 82.35% <100.00%> (ø) 23.00 <0.00> (ø)

@twz123
Copy link
Contributor Author

twz123 commented May 20, 2020

Updated the changelog.

@rhuss
Copy link
Collaborator

rhuss commented Jun 4, 2020

I think this PR is valid, as a user can change HOME but not user.home.

I wonder whether we need update/add some documentation, too ?

@sonarcloud
Copy link

sonarcloud bot commented Jun 4, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@rhuss
Copy link
Collaborator

rhuss commented Aug 29, 2020

user.home is not only used inDockerFileUtil but also in e.g VolumeBindUtil and also in DockerConnectionDetector. While I'm positive to this change I think we should also update all other places where user.home is used as the location of the home directory.

@twz123 anything you would like to tackle, too ? (sorry for the long delay, the intervals when i'm able to work on dmp get longer and longer ...)

@twz123
Copy link
Contributor Author

twz123 commented Aug 29, 2020

,@rhuss sure, I'll have a look, although this might take some time on my side, too 🙈

@rhuss
Copy link
Collaborator

rhuss commented Aug 29, 2020

,@rhuss sure, I'll have a look, although this might take some time on my side, too 🙈

thanks ! and no worries, we are operating on a slow time scale anyway (sorry, i feel guilty a bit here that everything is so slow here, but i'm working on a solution here)

@sonarcloud
Copy link

sonarcloud bot commented Sep 13, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@rohanKanojia
Copy link
Member

@twz123 : polite ping, Are you still working on this?

@rohanKanojia rohanKanojia added this to the 0.35.0 milestone Mar 7, 2021
@rhuss
Copy link
Collaborator

rhuss commented Mar 7, 2021

@rohanKanojia let's fix that on our own, fix shouldn't be hard. @twz123 hope this is ok for you when we take over the last steps.

The Java system property is filled by a syscall to determine the user's
home directory. This is not matching the typical implementation of the
docker and kubectl binaries, which prefer the HOME OS environment
variable over the syscall.
+ Update DockerConnectionDetector,VolumeBindingUtil with updated
  precedence for home OS environment variable
+ Move HOME OS env variable resolving logic to EnvUtil
@rohanKanojia rohanKanojia merged commit 48c1d06 into fabric8io:master Apr 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants