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

Add support for Docker credential providers which do not implement list command #902

Merged

Conversation

glebbash
Copy link
Contributor

Fixes #739

Copy link

netlify bot commented Jan 23, 2025

Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit 2ed4764
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-node/deploys/6792de19d226790008fab9ec
😎 Deploy Preview https://deploy-preview-902--testcontainers-node.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@cristianrgreco cristianrgreco added bug Something isn't working patch Backward compatible bug fix labels Jan 23, 2025
@cristianrgreco cristianrgreco changed the title fix: support docker-credential-gcr Add support for docker-credential-gcr Jan 24, 2025
@cristianrgreco
Copy link
Collaborator

cristianrgreco commented Jan 24, 2025

Hi @glebbash, you've added a test should not throw when list credentials command is not implemented, but it's not clear why not throwing when listing credentials is useful or required. Instead please add a test verifying an actual output, e.g a file containing GCR credentials now works.

@glebbash
Copy link
Contributor Author

I am not sure what you mean about a file with GCR credentials and what the expected output should be. As for reasons why this is required I think it's pretty clear - if command is not implemented it should just use defaults.

Adding a link to the issue in a comment before the condition could make it clearer but I just wanted to make the minimum amount of changes to support docker-credential-gcr.

@cristianrgreco
Copy link
Collaborator

cristianrgreco commented Jan 25, 2025

I mean, this PR supposedly fixes the resolution of GCR credentials, could you show that? A test showing that something returns an empty object doesn't show that

@glebbash
Copy link
Contributor Author

glebbash commented Jan 25, 2025

This PR does not fix resolution of GCR credentials. What it does is make testcontainers usable for whoever happens to be using docker-credential-gcr. I really don't care about credentials and I don't want to dig into this repo to figure out all the details.

I was patching the lib's compiled JS source to make it not throw the An error occurred listing credentials error. And here I copied the changes to the TypeScript source.

I don't know if this supports private GCR registries or whatever, the only thing I know is that it fixes the issue described in #739 which has 5 +1s so I don't think that it's just my setup that is affected.

@glebbash
Copy link
Contributor Author

@cristianrgreco if you have a better test in mind feel free to update the PR or fix this issue in different PR.

@cristianrgreco
Copy link
Collaborator

I think it's my misunderstanding TBH. The PR said it added support for docker-credential-gcr so I assumed it would support resolving GCR credentials. I didn't realise that supporting docker-credential-gcr actually involves just not throwing when running the list command 😄

I really don't care about credentials and I don't want to dig into this repo to figure out all the details.

Thought this was a bit rude TBH. I work on this project in my free time. If I propose a suggestion and the response is effectively "I don't care and I can't be asked to find out", it doesn't particularly motivate me to work on it.

the only thing I know is that it fixes the issue described in #739 which has 5 +1s so I don't think that it's just my setup that is affected

This project has a lot of moving parts and 2M downloads/month. Five +1s on a year old issue isn't exactly concerning, but thanks for the contribution.

if you have a better test in mind feel free to update the PR or fix this issue in different PR.

Thanks for the advice 😅

@cristianrgreco cristianrgreco changed the title Add support for docker-credential-gcr Add support for Docker credential providers which do not implement list command Jan 25, 2025
@cristianrgreco cristianrgreco merged commit f68ffbd into testcontainers:main Jan 25, 2025
191 checks passed
@glebbash
Copy link
Contributor Author

Definitely did not intend to be rude. Was just getting trying to describe the situation in multiple ways :)

And thanks for your work on the lib 👍

@glebbash glebbash deleted the fix/support-docker-credential-gcr branch January 27, 2025 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working patch Backward compatible bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An error occurred listing credentials with docker-credential-gcr
2 participants