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

Fix parsing registry from env variable to work with trailing slash #748

Conversation

ilia-beliaev-miro
Copy link
Contributor

Fixed the trailing slash in registry in env variable parsing behavior to work as specified in documentation.

Copy link

netlify bot commented Apr 3, 2024

Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit 177c7e6
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-node/deploys/660eb9f64c66000008dbdd11
😎 Deploy Preview https://deploy-preview-748--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 Apr 3, 2024
@cristianrgreco cristianrgreco changed the title Fixed parsing registry from env variable to work with trailing slash Fix parsing registry from env variable to work with trailing slash Apr 3, 2024
@cristianrgreco
Copy link
Collaborator

@ilia-beliaev-miro do you know if this PR resolves #747 in its entirety?

@ilia-beliaev-miro
Copy link
Contributor Author

@ilia-beliaev-miro do you know if this PR resolves #747 in its entirety?

Hey @cristianrgreco, I can't test it with their setup. Maybe @ jonesbusy could check it?

@jonesbusy
Copy link

I don't think it resolve it. I was trying to remove the trailing slash from the env vars and the status was the same. Was hanging on the lock. Same if I remove the env vars (for the 10.8)

While this PR will avoid creating double slash, it doesn't fix the issues I have.

Right now I cannot explain further those regression with 10.7 and 10.8. Also why using same setup it work with testcontainer java et .net.

Using 10.7 which doesn't use the env vars, error is that the "Started" log message is not received. Which made me think it was related to changes on the wait conditions.

@ilia-beliaev-miro
Copy link
Contributor Author

I found that it doesn't work with registries that have a path - auth config is then not found. Need to split the path part from the domain part.

@cristianrgreco
Copy link
Collaborator

@ilia-beliaev-miro is this PR ok to merge/release?

@ilia-beliaev-miro
Copy link
Contributor Author

@ilia-beliaev-miro is this PR ok to merge/release?

@cristianrgreco I am working on fixing the problem with the authentication with registry. Hope to push it in a couple of hours.

…ustom registry. Now the domain part is properly separated from the path part of the image substitution prefix
@ilia-beliaev-miro
Copy link
Contributor Author

Hey @cristianrgreco now it should be fine.

@cristianrgreco cristianrgreco merged commit e56ec2b into testcontainers:main Apr 5, 2024
118 checks passed
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.

None yet

3 participants