Devided docker.sh script to smaller onse by jhryniuk · Pull Request #23 · PGSSoft/php-docker-skeleton

jhryniuk · 2017-04-21T08:42:36Z

No description provided.

partikus

Multiple files approach looks nice but I noticed few improvements:

each file should indicate intention of file commands e.g. from build.sh to buildImages.sh
there is a lot of workarounds like cd && __DIR which are not clear
variables defined & used only under function block should have local scope see more
each file should apply SRP

I'm looking forward to improvements to merge it.

partikus · 2017-04-22T06:15:07Z

docker.sh

-    docker-compose -f docker-compose.yml -f docker-compose.local.yml kill > /dev/null 2>&1
-    docker-compose -f docker-compose.yml -f docker-compose.local.yml rm -f -v > /dev/null 2>&1
-}
+for FILE in $(ls "${__DIR}/bash") ; do


if you walk through the file lists you're not sure of order. this loop should be replaced by predefined list

Order is not important because those scripts contain only functions.
Calling those functions is below in this file, when everything is included.

partikus · 2017-04-22T06:16:05Z

docker.sh

-echo "User ID: $USERID";
-echo -e "\nIMAGE VERSION: $APP_NAME:$APP_VERSION\n";
-declare -i BUILD_STATUS=0;
+__DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"


Why did you choose such naming convention __DIR ?

I treated location as a magic variable
It is good to set this variable as a unique among of all variables.
@dominikpgs @tarnawski @partikus what do you think about that?

partikus · 2017-04-22T06:16:55Z

bash/variable.sh

@@ -0,0 +1,15 @@
+#!/usr/bin/env bash
+
+export PROJECT_NAME=$(cat "${__DIR}/.project_name")


where __DIR is defined? in another file but it could be confusing

partikus · 2017-04-22T06:17:43Z

bash/run.sh

+function runInBackground {
+    export IMAGE_VERSION=$1
+
+    cd "${__DIR}"


is there better option to run below commands under __DIR?

yes there is, but code will lose on readability.

partikus · 2017-04-25T12:15:14Z

bash/runBuild.sh

+    DIR=$1
+    export IMAGE_VERSION=$2
+
+    cd "${DIR}"


can not we change directory globally in docker.sh ?

partikus · 2017-04-25T12:18:50Z

docker.sh

+    'checkContainerExists.sh'
+    'runBuild.sh'
+    'runInBackground.sh'
+    'setupVariable.sh'


is it possible to rename this file to defineVariables.sh

jhryniuk · 2017-04-28T06:03:45Z

@partikus @dominikpgs ping

partikus · 2017-05-01T10:06:23Z

@jhryniuk rebase please

partikus · 2017-07-18T18:34:18Z

Is it still relevant? Is it necessary to split docker.sh into few smaller one?
I think refactoring of docker.sh script is necessary but I have no ideas what we could improve.

Let's discuss.

jhryniuk · 2017-07-20T21:07:05Z

I think that we can divide this file on basic sections.
Form example:

file with configuration of variables,
script with basic functions,
main script which use functions from script with functions and variables from configuration file.

It is just proposition.
I admit that I'd divide this scrip on to many files.

partikus suggested changes Apr 22, 2017

View reviewed changes

jhryniuk force-pushed the feature/split-bash branch from 23dbd58 to 9158b54 Compare April 24, 2017 07:37

partikus suggested changes Apr 25, 2017

View reviewed changes

jhryniuk added 2 commits May 2, 2017 10:12

Devided docker.sh script to smaller onse

7505ec6

Rebase

fb51702

jhryniuk force-pushed the feature/split-bash branch from 7482730 to fb51702 Compare May 2, 2017 08:29

		@@ -0,0 +1,15 @@
		#!/usr/bin/env bash

		export PROJECT_NAME=$(cat "${__DIR}/.project_name")

Conversation

jhryniuk commented Apr 21, 2017

Uh oh!

partikus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhryniuk Apr 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhryniuk commented Apr 28, 2017

Uh oh!

partikus commented May 1, 2017

Uh oh!

partikus commented Jul 18, 2017

Uh oh!

jhryniuk commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jhryniuk Apr 22, 2017 •

edited

Loading

jhryniuk commented Jul 20, 2017 •

edited

Loading