-
Notifications
You must be signed in to change notification settings - Fork 771
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 kompose.service.expose.tls-secret #896
Conversation
Tests pass (despite us having a hard barrier-to-entry to creating them, haha), code looks great, name makes sense. This LGTM. 👍 thanks a lot @Code0x58 ! Edit: So after looking at the code. Correct me if I'm wrong, but this essentially grabs a secret from Kubernetes from whatever name you set it as? (for this example, The problem with this is that it isn't exactly 1-1 conversion from Docker Compose to Kubernetes. You would already have to have a secret setup on Kubernetes in order to set this up, which doesn't fit with the whole "convert 1-1 from docker-compose to kubernetes". I have no problem merging this in, but this makes #296 a higher priority now, as ideally, you'd be able to define this secret within Docker Compose and then it's automatically created within Kubernetes (without having to define it manually), does that make sense? Let me know your thoughts 👍 |
Thanks, I definitely made use of CI on here while working out the tests - sempahore shows 12 builds. I think ingress TLS secrets won't map nicely between docker-compose/swarm and Kubernetes as it would be an application concern in compose/swam, so it will probably always be stuck in labels/workarounds like this. Pod I image that down the line when compose secrets are supported, you could specify your secret within the compose file, then refer to it in this label without any additional work - so this should be a stable approach. You may want to check that this refers to a defined secret, which could be an external one to work as it does now. |
Yeah, there's absolutely no way to map it correctly without doing something really hacky. I agree that labels are the right way to go. This LGTM 👍 Thanks for clarifying regarding the secret and TLS. IMO this should have another review. @kadel @containscafeine @surajnarwade can you have a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Mergin' thanks @Code0x58 ! |
No description provided.