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 config templating #111

Merged
merged 1 commit into from
Feb 13, 2024
Merged

Conversation

saltydk
Copy link
Contributor

@saltydk saltydk commented Dec 13, 2023

Suggestion for resolving #109 whilst keeping IPv6 functionality.

@sammcj
Copy link
Contributor

sammcj commented Jan 31, 2024

Any update on getting this merged @Tecnativa?

@pedrobaeza
Copy link
Member

I don't get why you need all the .template stuff for the IPv6 thing.

@saltydk
Copy link
Contributor Author

saltydk commented Jan 31, 2024

I don't get why you need all the .template stuff for the IPv6 thing.

Because of the linked issue while I cannot reproduce it personally there seems to be cases where haproxy doesn't start, guessing docker swarm edge case.

@pedrobaeza
Copy link
Member

Yeah, but for this I think there's no need of having 2 files. Just inject the environment variable on the file. Another think I don't like is the extra added package.

@saltydk
Copy link
Contributor Author

saltydk commented Jan 31, 2024

As far as I know haproxy doesn't allow templating in bind, if that was what you were going for.

@pedrobaeza
Copy link
Member

OK, what about the extra package?

@saltydk
Copy link
Contributor Author

saltydk commented Jan 31, 2024

The alternative sed solution is posted in the issue but didn't get any feedback on which was preferred.

@pedrobaeza
Copy link
Member

Then please proceed with one not adding more load to the container.

@saltydk
Copy link
Contributor Author

saltydk commented Jan 31, 2024

Done.

@saltydk
Copy link
Contributor Author

saltydk commented Jan 31, 2024

Removed the -f flag so it matches the current entrypoint logic https://github.com/docker-library/haproxy/blob/master/2.2/alpine/docker-entrypoint.sh

@pedrobaeza
Copy link
Member

Is the CI fault related?

@saltydk
Copy link
Contributor Author

saltydk commented Jan 31, 2024

It has error'ed for a while, don't think it is related to any of this PR.

@pedrobaeza
Copy link
Member

@josep-tecnativa please check how to fix the CI in this project.

@saltydk saltydk force-pushed the feature/templating branch 2 times, most recently from f22ca91 to efca8bc Compare January 31, 2024 15:30
@saltydk
Copy link
Contributor Author

saltydk commented Jan 31, 2024

Yeah, I can't find any faults. Should I change the default to be IPv6 enabled? Currently it fallbacks to IPv4 only.

@saltydk
Copy link
Contributor Author

saltydk commented Jan 31, 2024

Given that the CI throws a IPv6 error:

Stderr: | error during connect: Get "http://localhost:32768/v1.24/containers/ebcc16f045797cd6b7c937952c84bfbdfa9d034a206d0997162ceae82a75a0f6/json": read tcp [::1]:48582->[::1]:32768: read: connection reset by peer

@pedrobaeza
Copy link
Member

Try to see. I think the IPv6 support was merged because a green CI.

@saltydk
Copy link
Contributor Author

saltydk commented Jan 31, 2024

It is failing to install the test

The current project could not be installed: No file/folder found for package docker-socket-proxy
If you do not want to install the current project use --no-root

@saltydk
Copy link
Contributor Author

saltydk commented Jan 31, 2024

I've inverted the setting but it didn't change anything. The PR that I made last time didn't fail as a PR but failed once merged so something must have happened in-between.

@saltydk
Copy link
Contributor Author

saltydk commented Jan 31, 2024

Added back the -f flag since it doesn't work with the old entrypoint style. The image works fine when deployed on my box for what it is worth.

@saltydk
Copy link
Contributor Author

saltydk commented Jan 31, 2024

Made it a bit cleaner by replacing the built-in entrypoint. Still cannot fix the build-test.

@saltydk
Copy link
Contributor Author

saltydk commented Feb 1, 2024 via email

@josep-tecnativa
Copy link
Contributor

Check this out : #116 . I think that by running poetry update, the error is fixed.

@saltydk
Copy link
Contributor Author

saltydk commented Feb 1, 2024 via email

@pedrobaeza
Copy link
Member

Well, now it's just rebase this branch over current master

@saltydk saltydk reopened this Feb 1, 2024
@saltydk
Copy link
Contributor Author

saltydk commented Feb 1, 2024

Done.

@josep-tecnativa
Copy link
Contributor

@saltydk
Copy link
Contributor Author

saltydk commented Feb 13, 2024

No, it just replaces the script that was already present in the base image.

@saltydk
Copy link
Contributor Author

saltydk commented Feb 13, 2024

@josep-tecnativa
Copy link
Contributor

For reference: https://github.com/docker-library/haproxy/blob/2913aa63fe4594a441e41f13e2c820092064a032/2.2/alpine/Dockerfile#L100-L106

Understood, thank you for the clarification. From my side, it looks good, I'll merge it.

@josep-tecnativa josep-tecnativa merged commit 45f7fb0 into Tecnativa:master Feb 13, 2024
6 checks passed
@jscheytt
Copy link

Thank you @saltydk @pedrobaeza @josep-tecnativa for getting this merged! The problem I was encountering in #109 is solved by this if I set the env var DISABLE_IPV6: "1".

Maybe we could add some info about this to the README?

@pedrobaeza
Copy link
Member

Yes, can you please make a PR adding such information?

@jscheytt
Copy link

Sure: #119

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.

5 participants