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

use credential helper if defined #821

Merged
merged 2 commits into from
Nov 3, 2017
Merged

Conversation

denisa
Copy link
Contributor

@denisa denisa commented Jul 20, 2017

Signed-off-by: Denis N. Antonioli d_n_antonioli@yahoo.com

Fix issue #806.
In need of feedback, especially any idea on how to test this…

I have only performed happy-path tests manually until now.

@codecov
Copy link

codecov bot commented Jul 20, 2017

Codecov Report

Merging #821 into master will decrease coverage by 0.06%.
The diff coverage is 29.57%.

@@             Coverage Diff              @@
##             master     #821      +/-   ##
============================================
- Coverage     50.55%   50.49%   -0.07%     
- Complexity     1185     1196      +11     
============================================
  Files           137      138       +1     
  Lines          7040     7104      +64     
  Branches        937      947      +10     
============================================
+ Hits           3559     3587      +28     
- Misses         3175     3209      +34     
- Partials        306      308       +2
Impacted Files Coverage Δ Complexity Δ
...ric8/maven/docker/access/util/ExternalCommand.java 10.71% <0%> (+10.71%) 3 <0> (+3) ⬆️
...ric8/maven/docker/util/CredentialHelperClient.java 25% <25%> (ø) 3 <3> (?)
...o/fabric8/maven/docker/util/AuthConfigFactory.java 79% <55.55%> (-2.09%) 66 <2> (+5)

@rhuss
Copy link
Collaborator

rhuss commented Jul 31, 2017

Thanks ! I will have a look asap, but will be on PTO this week, so might take a bit. Please ping me, if I forget this after this week.

@denisa
Copy link
Contributor Author

denisa commented Sep 12, 2017

any chance of getting this reviewed in a neat future?

@rhuss
Copy link
Collaborator

rhuss commented Sep 14, 2017

sorry, still under full load with other stuff ;-( But I definitely consider this PR for the next release, which is not so far away. Hopefully on the weekend, but if not, it still might take three weeks as I'm fully booked with other work for the next time.

really sorry, but it's not forgotten ...

denisa and others added 2 commits November 3, 2017 12:02
Signed-off-by: Denis N. Antonioli <d_n_antonioli@yahoo.com>
@rhuss
Copy link
Collaborator

rhuss commented Nov 3, 2017

I rebased and made some minor updates.

Look good to me, lets merge it (sorry for the ultra long delay).

wrt testing: I think we could do this by including some shell scripts into the code which are written out to a tempdir during the test and then mock the .dockercfg read to point to this shell scripts which return expected JSON when called as external command. It's a bit involved and only works for certain environments (unix like), but might be worth to try. If you feel fancy, you can give it a try (I'm unfortunately still under high load so have no time for this :(

Thanks a lot ! (and sorry again, you can expect a release on the weekend)

@rhuss rhuss merged commit 97310fb into fabric8io:master Nov 3, 2017
@rhuss
Copy link
Collaborator

rhuss commented Nov 4, 2017

@denisa 0.23.0 is out. Thanks for your contribution !

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.

2 participants