reactioncommerce / docker-base

Apache License 2.0
4 stars 11 forks source link

refactor: DRY up all push and build scripts #5

Closed focusaurus closed 4 years ago

focusaurus commented 4 years ago

Sure that's easy enough to add. Will do.

focusaurus commented 4 years ago

OK now you give dockerfiles.sh a subcommand either build or push then an optional list of paths to dockerfiles. If you omit the path list it finds them all by naming convention in the repo.

aldeed commented 4 years ago

@focusaurus I tried:

./dockerfiles.sh build ./images/node-dev/10.16.3-v2/Dockerfile

And it worked except at the end it tagged incorrectly:

Successfully tagged reactioncommerce/images:node-dev

I think bumping the piece nums for name and tag vars would be the solution, but I wanted to check with you in case I'm doing the command differently from what you expected.

focusaurus commented 4 years ago

Ah crap it's breaking depending on whether args have leading "./" or not. Fixing...

On Thu, Nov 7, 2019 at 12:56 PM Eric Dobbertin notifications@github.com wrote:

@focusaurus https://github.com/focusaurus I tried:

./dockerfiles.sh build ./images/node-dev/10.16.3-v2/Dockerfile

And it worked except at the end it tagged incorrectly:

Successfully tagged reactioncommerce/images:node-dev

I think bumping the piece nums for name and tag vars would be the solution, but I wanted to check with you in case I'm doing the command differently from what you expected.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/reactioncommerce/docker-base/pull/5?email_source=notifications&email_token=AADVYSIRBESUHWA7CFJNSB3QSRXGDA5CNFSM4JJF4PD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDNTVSQ#issuecomment-551238346, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADVYSO3NJ5SP6LWDT5EW2LQSRXGDANCNFSM4JJF4PDQ .

focusaurus commented 4 years ago

OK switched it to grab directory path elements from the end backward which should be more robust against optional leading ./ prefix.

aldeed commented 4 years ago

That makes sense. I verified it works now with and without prefix, and with multiple paths with a combination of prefix and not. 👍