Rename tc-platform to tc-website and move httpd into its own image. #14
Conversation
docker/tc-website/docker-compose.yml
Outdated
| - "8009:8009" | ||
| extra_hosts: | ||
| - "env.topcoder.com:172.16.100.1" |
There was a problem hiding this comment.
May I ask why you have an IP address in the docker-compose?
Unless this is set by the docker daemon and is guaranteed to never change, it will not make the Dockerfile portable. If that is the case then it should be:
- configured in
env.shand; - mentioned explicitly in the README.md.
There was a problem hiding this comment.
We're using env.topcoder.com point to multiple container(cache and informix), the only way get on with this is point env.topcoder.com to host ip. It will be used by docker-compose to configure its network, see network section on docker-compose.yml.
There was a problem hiding this comment.
OK. One of the reviewers mentioned that they had issues with deployment on a different OS.
Were you able to test on more than one OS out of Linux, Windows and macOS?
Thanks.
There was a problem hiding this comment.
Schpotsky has confirmed it works on mac. see his comment on here https://software.topcoder.com/review/actions/ViewReview?rid=388296
here is the screenshoot
https://drive.google.com/open?id=0B1ZT_oEfOX1wSzh2ZGRERV9WODQ
I don't what other reviewer face on. He face same trouble on previous challenge either.
There was a problem hiding this comment.
I have a MacBook I can use to test but that should not hold me back from approving the pull request once I am able to test the changes requested on Windows and/or Linux.
Thanks.
docker/tc-website/README.md
Outdated
|
|
||
| ## Deployment | ||
| Update the configuration values in env.sh file from the submission: | ||
| * TC_PLATFORM_SRC_ROOT - the root directory of the tc-platform codebase, point to the tc-platform directory above |
There was a problem hiding this comment.
TC_PLATFORM_SRC_ROOTis a bit confusing. Perhaps we should rename it toTC_WEBSITE_HOME(similar toJAVA_HOMEandTOMCAT_HOME)?.DEPLOYMENT_DIR- please rename toJBOSS_DEPLOYMENT_DIRas used in my forum clarification: https://gist.github.com/sah2ed/3608337ee39dc96f6db302abf26f43f9#file-docker-compose-yml-L47
There was a problem hiding this comment.
TC_PLATFORM_SRC_ROOT contains not only tc-website. I think it's better use TC_PLATFORM_HOME
There was a problem hiding this comment.
For consistency, TC_WEBSITE_HOME is better since:
- the
external-artifacts,glueandsharedrepos use thetc-website-*prefix. - the Dockerfiles have been renamed to now use
tc-websiteinstead oftc-platform.
Besides, tc-platform is obscure since there is no Topcoder system that uses such a name ;-).
Many members know which systems direct-app and online-review refers to.
docker/tc-website/README.md
Outdated
| git clone https://github.com/appirio-tech/temp-maven-repo | ||
| ``` | ||
|
|
||
| ## Applying Patch |
There was a problem hiding this comment.
- The Applying Patch section is specifically for reviewers of the challenge and should not be in the README.md.
- The challenge explicitly requested for markdown formatted verification using screen shots instead you provided a PDF. PDF is a binary format and obviously doesn't play nice with git so please restore the former text in the Verification section below.
docker/tc-website/README.md
Outdated
|
|
||
| ## Checkout tc-website and all other repo | ||
| ``` | ||
| mkdir -p $HOME/tc-platform |
There was a problem hiding this comment.
Please refactor this line mkdir -p $HOME/tc-platform to this:
TC_WEBSITE_HOME=/home/tc/tc-platform
mkdir -p $TC_WEBSITE_HOME
docker/tc-website/build/build.sh
Outdated
| cp -f /root/files/TC.prod.ldap.keystore $JBOSS_HOME/bin | ||
| cp -f /root/files/resources/paymentRanges.xml $JBOSS_HOME/server/all/conf | ||
|
|
||
| # init code | ||
| cp -f /root/files/resources/ApplicationServer.properties /root/tc-platform/tc-website/resources | ||
| cp -f /root/files/resources/cache.properties /root/tc-platform/tc-website/resources | ||
| cp -f /root/files/resources/LDAP.properties /root/tc-platform/tc-website/resources | ||
| cp -f /root/files/distui/jboss-web.xml /root/tc-platform/tc-website/resources/distui |
There was a problem hiding this comment.
I don't see a corresponding update in the list of files updated in the pull request for tc-website ?
Shouldn't jboss-web.xml be updated?
Thanks.
There was a problem hiding this comment.
jboss-web.xml will be copied to jboss directory after deploying success. See last line of build.sh
So no need to change on tc-website
There was a problem hiding this comment.
Oh, no wonder I didn't see it. The previous approach of overwriting the file before executing the build is better.
The ant build targets should be the last build-related command in the Dockerfile since it is also used to verify continuous deployment of the tc-website on CircleCI.
There was a problem hiding this comment.
So you want to merge it to tc-website?
There was a problem hiding this comment.
Overwriting it with /root/files/distui/jboss-web.xml like was done before is fine. No need to merge it with tc-website.
| @@ -1,4 +1,4 @@ | |||
| FROM tc-website:base | |||
| FROM appiriodevops/tc-website:base | |||
There was a problem hiding this comment.
FYI, I've corrected the name of the base docker image.
sah2ed
changed the title
|
@skyhit I have no preference especially since the docker images are meant to replace the Amazon VMs which have been using the |
No description provided.