-
Notifications
You must be signed in to change notification settings - Fork 35
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 ExposedPorts to the Image config #113
Conversation
For a test asp.net core container, we now have ExposedPorts: "ExposedPorts": {
"80/tcp": {}
} and we can see the ports being automatically bound when ➜ docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
a41168f7eac0 container-app-tres:0.0.4 "/app/container-app-…" 5 seconds ago Up 4 seconds 0.0.0.0:49153->80/tcp wonderful_faraday and indeed, navigating to that port shows the app. |
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.
overall lgtm, my biggest gripe is testing. Won't block on it if we need to merge & create an issue for that
Microsoft.NET.Build.Containers/build/Microsoft.NET.Build.Containers.targets
Outdated
Show resolved
Hide resolved
I made significant enough changes to the port parsing to warrant a re-review, but I did add a crap ton of testing to the core logic. Also went ham on the user-facing errors. |
Large enough changes to warrant re-review.
Co-authored-by: Rainer Sigwald <[email protected]>
Rebased this on top of latest main - the only actual change was to flow ports through the |
@benvillalobos when this merges we'll want to take care and make sure that the new tool task also handles writing port arguments to the CLI. |
Co-authored-by: Rainer Sigwald <[email protected]>
This is a fairly mechanical transformation. Things remaining to do: