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

Credential helpers #729

Merged
merged 16 commits into from
Jun 16, 2018
Merged

Credential helpers #729

merged 16 commits into from
Jun 16, 2018

Conversation

TheIndifferent
Copy link
Contributor

Previous PR seems to be stalled, here is the same code with the if condition fix to invoke credentials helper if no credentials were found, plus merged with current master.

This functionality is tested on Docker-for-Mac and works fine. Unfortunately I am not able to test it on Windows, but it would be awesome if this was included in next release (as per "release early, release often") and maybe the Windows fix will arrive later when someone will have a chance to test/fix it?

See #647 for previous discussions.

@TheIndifferent
Copy link
Contributor Author

Ok, didn't expect it to fail the build, sorry. Working on it.

@rnorth
Copy link
Member

rnorth commented Jun 4, 2018

@TheIndifferent thanks - work isn't stalled per se, but I'm very busy 😄.

I'm very grateful for your input on this, and will close my original PR.

@TheIndifferent
Copy link
Contributor Author

Ok, so quick update: the assumption about incorrect logic there is completely wrong. I am testing with gcr.io at the moment, it behaves little differently than expected, so I will try to add one more unit test and make everything work.

@TheIndifferent
Copy link
Contributor Author

Please see the latest commit, had to reorganize the code a little bit, but now tested it with gcloud docker --authorize-only and with empty auth section but with helper present.

@TheIndifferent
Copy link
Contributor Author

Cannot really understand why Travis build is failing, does not seem to be related. Could someone please take a look and either rebuild or point me to the actual error to fix?

@bsideup
Copy link
Member

bsideup commented Jun 5, 2018

@TheIndifferent could be related to #731

@bsideup
Copy link
Member

bsideup commented Jun 5, 2018

@TheIndifferent err, no, it's actually javadoc task (you can also run it locally)

@TheIndifferent
Copy link
Contributor Author

@bsideup thank you for the pointer, fixed it, it passes now. Could you please take a look at current version of RegistryAuthLocator if it looks sane?

Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

I'm pretty happy with this, but I'm biased. Thank you for picking this up and making it better, @TheIndifferent.

I've put a couple of comments for minor things; I'd be happy to tweak and push to your branch if you want, and if I have permission.

@bsideup, @kiview, how would you feel about releasing this just for Linux and Mac users, and coming back to it for Windows? I'd just like to get it shipped, and it pains me to be holding it back.

@@ -22,7 +22,10 @@ All notable changes to this project will be documented in this file.

## [1.7.2] - 2018-04-30

- Add support for private repositories using docker credential stores/helpers (fixes [\#567](https://github.com/testcontainers/testcontainers-java/issues/567))
Copy link
Member

Choose a reason for hiding this comment

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

Please could you shift this up to the UNRELEASED section?

Also, it looks like this same line appears (more or less the same) three times - merge issue?

I think that for now we should draw a line in the sand and release this for Mac and Linux - could we say so in the changelog entry?

*/
public AuthConfig lookupAuthConfig(DockerImageName dockerImageName) {
log.debug("Looking up auth config for image: {}", dockerImageName);

Copy link
Member

Choose a reason for hiding this comment

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

We should probably have a check here that we're not running on Windows and fall back to normal (current) behaviour.

@TheIndifferent
Copy link
Contributor Author

@rnorth got it, I will address the comment this weekend, sorry for the delay.

@rnorth
Copy link
Member

rnorth commented Jun 15, 2018

I have a moment now - I'll update this.

@rnorth
Copy link
Member

rnorth commented Jun 15, 2018

I've updated and rebased on top of the 1.8.0 release. I'll let CI run but from my perspective this should be good to go. I also raised #756 so that we don't forget to resolve this for Windows users.

@rnorth rnorth merged commit 92266b2 into testcontainers:master Jun 16, 2018
@rnorth
Copy link
Member

rnorth commented Jun 16, 2018

Hi @TheIndifferent, I have merged this into master. Thanks for your help on this!

@TheIndifferent
Copy link
Contributor Author

@rnorth thank you for picking this up and merging! Can't wait for next release to try it out for real :)

@rnorth
Copy link
Member

rnorth commented Jul 12, 2018

Released in 1.8.1! 🎉

Thanks for your work on this, @TheIndifferent!

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