-
Notifications
You must be signed in to change notification settings - Fork 341
Add WCF sample #136
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 WCF sample #136
Conversation
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.
The primary samples readme should be updated to reference the WCF samples - https://github.com/Microsoft/dotnet-framework-docker/blob/master/samples/README.md#building-net-framework-apps-with-docker
samples/wcfapp/WcfClient/App.config
Outdated
<?xml version="1.0" encoding="utf-8" ?> | ||
<configuration> | ||
<startup> | ||
<supportedRuntime version="v4.0" sku=".NETFramework,Version=v4.6.2" /> |
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.
I think it would be best to update the samples to be using the latest FX version - 4.7.2
@@ -0,0 +1,5 @@ | |||
FROM microsoft/wcf:4.6.2 | |||
RUN windows\system32\inetsrv\appcmd.exe set app 'Default Web Site/' /enabledProtocols:"http,net.tcp" | |||
EXPOSE 808 |
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.
Is the EXPOSE necessary since it is in the base image?
@@ -0,0 +1,5 @@ | |||
FROM microsoft/wcf:4.6.2 | |||
RUN windows\system32\inetsrv\appcmd.exe set app 'Default Web Site/' /enabledProtocols:"http,net.tcp" |
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.
Is this necessary since it is in the base image?
@@ -0,0 +1,6 @@ | |||
FROM microsoft/dotnet-framework:4.6.2 |
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.
What would it take to have a multi-stage Dockerfile that builds the project with one stage and produces the "runtime" image in another? Can you make use of microsoft/dotnet-framework:4.7.2-sdk to build? We should be supporting containerized builds.
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.
@MichaelSimons In this sample, after WCF services are running in the container, users should be able to run WCF client against them. If the projects are build inside container, users still need to build the project locally to have WcfClient.exe. Am I understanding it correctly?
samples/wcfapp/README.md
Outdated
@@ -0,0 +1,59 @@ | |||
# WCF Docker Container Samples |
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.
I would like to see us build and push at least one sample to microsoft/dotnet-framework-samples so that someone can just pull and try something. e.g. https://github.com/Microsoft/dotnet-framework-docker/blob/master/samples/dotnetapp/README.md#try-a-pre-built-net-framework-docker-image
To do this you can add the appropriate entries in https://github.com/Microsoft/dotnet-framework-docker/blob/master/manifest.samples.json. I can help you with this if you have questions.
samples/wcfapp/README.md
Outdated
# WCF Docker Container Samples | ||
These samples demonstrate how to dockerize WCF services, either IIS-hosted or self-hosted. A simple "hello world" service contract is used in all samples for both HTTP and NET.TCP transport bindings. | ||
|
||
## Prepare Your Environment |
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.
I think the best UX would be for the format of this readme to mirror the other samples where it make sense. As an example, there should be a section that mirrors https://github.com/Microsoft/dotnet-framework-docker/blob/master/samples/dotnetapp/README.md#getting-the-sample.
samples/wcfapp/README.md
Outdated
``` | ||
C:\wcfapp\WcfServiceConsoleApp>docker run -d -p 80:80 -p 808:808 --name myservice3 wcfservice:self-hosted | ||
``` | ||
Then for the WCF client to connect to the service, we need to set the `host` to be the IP address (or DNS name) of the container host machine (instead of the IP address of the container instance). The rest will be the same to start the WCF client. |
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.
At the end, it would be beneficial to link to additional resources - see https://github.com/Microsoft/dotnet-framework-docker/blob/master/samples/dotnetapp/README.md#net-resources.
@@ -0,0 +1,8 @@ | |||
FROM microsoft/dotnet-framework:4.7.2-sdk AS build | |||
WORKDIR /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.
The nested /app/WcfClient/ path seems unnecessary in the context of Docker. What do you think about eliminating the nesting? It could be simplified to just /app
or /WcfClient
.
samples/wcfapp/Dockerfile.client
Outdated
WORKDIR /app | ||
COPY WcfClient/. ./WcfClient/ | ||
WORKDIR /app/WcfClient | ||
RUN msbuild |
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.
I think it would be better to illustrate a production scenario. In that vein using /p:Configuration=Release
feels appropriate.
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.
You may also choose to specify the output dir as done in https://github.com/Microsoft/dotnet-framework-docker/blob/master/samples/dotnetapp/Dockerfile#L29
samples/wcfapp/Dockerfile.client
Outdated
RUN msbuild | ||
|
||
FROM microsoft/dotnet-framework:4.7.2-runtime | ||
COPY --from=build /app/WcfClient/bin/Debug/ ./ |
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.
Any particular reason the app is copied to the root versus an app directory?
samples/wcfapp/README.md
Outdated
Now that we have WCF services running in containers. Let's run the WCF client against them. Run commands below to build the container image with name `wcfclient`. | ||
``` | ||
docker build --pull -t wcfclient -f Dockerfile.client . | ||
docker run -it --name myclient wcfclient |
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.
Have you considered simplifying these steps by setting the env var with the run command?
docker run --rm -e HOST=172.23.70.146 wcfclient
samples/wcfapp/README.md
Outdated
Now that we have WCF services running in containers. Let's run the WCF client against them. Run commands below to build the container image with name `wcfclient`. | ||
``` | ||
docker build --pull -t wcfclient -f Dockerfile.client . | ||
docker run -it --name myclient wcfclient |
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.
Our samples should be encouraging the use of --rm
to promote cleaning up the Docker resources when done. I think this usage is a good candidate for using --rm
.
samples/wcfapp/README.md
Outdated
C:\wcfapp\WcfClientNetCore>docker inspect --format="{{.NetworkSettings.Networks.nat.IPAddress}}" myservice2 | ||
172.23.69.75 | ||
``` | ||
### Build a Container to run WCF Client against the Service |
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.
What do you think about adding a Docker Compose file that will spin up both the client and service? It could define a service link and then you can use the host name instead of having to know the IP. You may need one each for the IIS and self hosted scenarios.
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.
@MichaelSimons I'm not very familiar with Docker-compose, I'll devote some time to this later today.
samples/wcfapp/README.md
Outdated
cd samples | ||
cd wcfapp | ||
docker build --pull -t wcfservice:iis-hosted -f Dockerfile.web . | ||
docker run -d --name myservice1 wcfservice:iis-hosted |
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.
Consider using a more descriptive container name e.g. iishostedwcfservice
. Same comment applies to the self-hosted section.
@MichaelSimons I updated the manifest.samples.json file for WCF prebuilt images, is there anyway I can test these two images? |
@yujayee, if you can push your changes to an upstream branch, I can queue a test build for you. |
@MichaelSimons Thanks, I just pushed the changes to a new branch wcftestbuild. |
@yujayee - I kicked off a test build, and pushed the images to https://hub.docker.com/r/msimons/dotnet-framework-samples/tags/ |
FYI - the 1803 samples didn't build because the branch is out of date. You'll want to merge in the latest from master at some point. |
samples/wcfapp/Dockerfile.client
Outdated
FROM microsoft/dotnet-framework:4.7.2-runtime AS runtime | ||
WORKDIR /app | ||
COPY --from=build /app/bin/Release . | ||
CMD WcfClient.exe |
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.
I wonder if this shouldn't be set as the ENTRYPOINT instead. It is the only purpose of the image. Same comment applies to Dockerfile.console
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.
Looks good. Just a couple minor comments
samples/wcfapp/README.md
Outdated
Then for the WCF client to connect to the service, we need to set the `host` to be the IP address (or DNS name) of the container host machine (instead of the IP address of the container instance). The rest will be the same to start the WCF client. | ||
|
||
## Build and run the sample with Docker Compose | ||
[Docker Compose](https://docs.docker.com/compose/overview/) is a tool for defining and running multi-container Docker applications. In this sample, we also added YML files to configure WCF server and client applications. You can directly start and run service and client applications without finding IP address of service container. |
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.
What do you think about changing without finding IP address of service container
to without having to any work to hookup the client to the WCF service
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 - Thanks for working on this!
@yujayee - As soon as I merged, I realized one more thing needs to be updated. Can you update the readme posted to microsoft/dotnet-framework-samples? Just create a PR to update the readme and once merged, the build infra will automatically update the readme on DockerHub. Thanks |
#129
I just copied the WCF sample from old repo to here. Please let me know if there are any new formats we need to follow. Thanks!
@MichaelSimons @richlander @Lxiamail