IBMStockTrader / trader

UI microservice for the Stock Trader app
Apache License 2.0
24 stars 63 forks source link

Added OCP Build Pipeline #6

Closed maxveit closed 4 years ago

maxveit commented 4 years ago
jwalcorn commented 4 years ago

Did you delete the Apache license comments from the Dockerfile? Also, I don't think we need those RUN ls lines anymore - don't want to end up with extra layers in the Docker image because of that.

jwalcorn commented 4 years ago

Also, it looks like the change history shows edits to the deploy.yaml, then a rename - so there's a merge conflict due to that deploy.yaml. can you make it so you haven't touched deploy.yaml, but rather simply created a brand new file with the DeploymentConfig?

jwalcorn commented 4 years ago

Sorry to keep piling on, but we probably don't want to hard-code the namespace for the ImageStream to be jenkins either, as that is a misleading name since Jenkins isn't actually being used here. What's the downside to just using the currently configured namespace (often stock-trader - whatever oc login gives you, or you explicitly switch to via oc project), with appropriately named objects?

Oh, and I see that change you made to add "demo :)" to that doc file during your live demo to me is in there.... ;)

rtclauss commented 4 years ago

One comment, if you're using GHE or a private repo that is secured and requires ssh keys to authenticate, make sure you use the git@github.com:.... pattern when entering the GIT_SOURCE_URL

jwalcorn commented 4 years ago

Used new PR (#7) to do this, so closing this one out.