If defined use environment variable to load config vars

[gbalabasquer]

Not sure if the cleanest way to define the config files, please let me know your thoughts. Somehow we need to pass the variable via params to the common.sh. Other option would be setting this up inside a function, but in that case, we would need to include common.sh then call this function, and then another line to define the config file variable assignment.

[gbalabasquer]

To give more context, this is a requirement from @konstantinzolotarev so he can edit config variables according to the users input without having to manage physical config files. The idea is he will be taking one config file as a base, importing it in: $TDDS_CONFIG_VALUES, then applying the user customization, and running the script from there.

[gbalabasquer]

btw, @icetan just realized we are missing some absolutes path in base-deploy. Not sure how is working without them in the nix version (or if actually working). But anyway as soon as this gets merged, I'll take care of them and open another PR.

[asymmetric]

I applied the suggestions by accident, sorry! Feel free to revert!

[icetan]

btw, @icetan just realized we are missing some absolutes path in base-deploy. Not sure how is working without them in the nix version (or if actually working). But anyway as soon as this gets merged, I'll take care of them and open another PR.

I guess you are referring to the dss-deploy script calls (e.g. ./bin/deploy-core)?

They are actually also handled by a regex which removes the ./bin/ prefix. Because dss-deploy is also Nixified and added to tdds's context, the scripts are callable without a path prefix.

Will make a PR to replace this with a env var.

[gbalabasquer]

btw, @icetan just realized we are missing some absolutes path in base-deploy. Not sure how is working without them in the nix version (or if actually working). But anyway as soon as this gets merged, I'll take care of them and open another PR.

I guess you are referring to the dss-deploy script calls (e.g. ./bin/deploy-core)?

They are actually also handled by a regex which removes the ./bin/ prefix. Because dss-deploy is also Nixified and added to tdds's context, the scripts are callable without a path prefix.

Will make a PR to replace this with a env var.

Ahh ok that explains why works. But yeah, if we can make all with ENV VARS will look better I think.

[gbalabasquer]

@gbalabasquer to separate the concerns of common.sh I think your suggestion of having a function to load or set the config is better.

We already do e.g. export CONFIG_STEP=kovan to set the config step. Instead this would be calling setConfigStep kovan or setOutConfig which is more explicit and doesn't hide complex behavior inside common.sh. This also makes common.sh usable for scripts that don't want to have config side effects.

@icetan So I was imaging some function like this in ´common.sh`:

setConfigStep() {
    CONFIG_STEP=${CONFIG_STEP:-"$1"}

    # Only executes if called from the initial script
    if [[ "$CONFIG_STEP" == "$1" ]]; then
        # Clean out directory
        rm -rf "$OUT_DIR" && mkdir "$OUT_DIR"
        # If environment variable exists bring the values from there, otherwise use the config file
        if [[ -n "$TDDS_CONFIG_VALUES" ]]; then
            echo "$TDDS_CONFIG_VALUES" > "$CONFIG_FILE"
        else
            cp "$CONFIG_DIR/$CONFIG_STEP.json" "$CONFIG_FILE"
        fi
    fi
    echo "$CONFIG_STEP"
}

Then in all the steps scripts + kovan have something like this:

# shellcheck source=lib/common.sh
. "${LIB_DIR:-$(cd "${0%/*}/lib"&&pwd)}/common.sh"
CONFIG_STEP=$(setConfigStep "step-1")
export CONFIG_STEP

Is it similar og how you were thinking about it? Other option would be exporting the $CONFIG_STEP in the function and calling it with the .. But not sure if it would be a bit dirty solution.

[icetan]

@gbalabasquer I think your suggestion with setConfigStep is good.

You can definitely do export CONFIG_STEP inside the function, bash functions do not execute in a subshell and only variables declared with local are scoped, so this should work fine :)

You would then only need to call it like so:

# shellcheck source=lib/common.sh
. "${LIB_DIR:-$(cd "${0%/*}/lib"&&pwd)}/common.sh"
setConfigStep step-1

Note that sourcing (.) functions is not possible because they are not files.

[asymmetric]

If export CONFIG_STEP=$(configStep step-1) works, I think it's cleaner than having the function affect the caller's state.

[icetan]

If export CONFIG_STEP=$(configStep step-1) works, I think it's cleaner than having the function affect the caller's state.

I agree, that makes it more clear what is going on.

But you could also argue that abstracting away the direct manipulation of CONFIG_STEP and instead use setConfigStep and getConfigStep would make it cleaner.

configStep would still have to check what CONFIG_STEP was previously set to, which makes controlling CONFIG_STEP outside the abstraction of configStep a bit leaky.

[gbalabasquer]

Note that sourcing (.) functions is not possible because they are not files.

Understood

I agree, that makes it more clear what is going on.

But you could also argue that abstracting away the direct manipulation of CONFIG_STEP and instead use setConfigStep and getConfigStep would make it cleaner.

configStep would still have to check what CONFIG_STEP was previously set to, which makes controlling CONFIG_STEP outside the abstraction of configStep a bit leaky.

I'm leaving it as it is for now. We can define later if we wanna change to the export inside the function.

[gbalabasquer]

I'm merging this PR so we can move on. We will be evaluating #34, #35 and the extra needed things later.