Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to use local services area via optional parameter #1269

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vkuznet
Copy link
Collaborator

@vkuznet vkuznet commented Dec 1, 2022

No description provided.

Copy link
Contributor

@amaltaro amaltaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valentin, I have a general comment on this script which is, whenever I have to use it, I don't pass the <env> option and I simply rely on it to be self-discovered by the script and my environment. In that scenario, we might have a mix up of parameters 3 and 4 (aka. positional arguments).

So, I think to have a clear and solid script option, we should either:
a) remove the env option and always rely on the environment for the correct execution
b) or, make it keyword arguments, e.g. -t <tag> -e <env> -p <services_path> etc

What do you think? @muhammadimranfarooqi and @arooshap might have a different view as well.

@muhammadimranfarooqi
Copy link
Contributor

Thanks @vkuznet

Changes look fine. @amaltaro I don't think this scripts works without env as all three parameters are compulsory to run the script

@amaltaro
Copy link
Contributor

amaltaro commented Dec 2, 2022

@muhammadimranfarooqi from line 4, we need to provide >= 2 parameters. Thus, running it only with the service name and the tag works just fine.

@@ -2,7 +2,7 @@
# helper script to deploy given service with given tag to k8s infrastructure

if [ $# -lt 2 ]; then
echo "The required parameters for service and tag are missing. Please use deploy-srv.sh <service> <tag> <env> "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amaltaro Yes, this can cause a mixup of parameters 3 and 4. I think rather than removing the env variable entirely, we can make the script take keyword arguments because when you're doing multiple deployments at the same time, it helps to make sure that you are deploying in the correct environment.

@vkuznet
Copy link
Collaborator Author

vkuznet commented Dec 2, 2022

I implemented script variables, please see updated code. For simplify here is a stand-alone script to demonstrate how it works (I name this script as t.sh):

#!/bin/bash

function Usage()
{
     echo "t.sh -s <service> -t <tag> -e <env> -d <optional services directory>"
     exit 1;
}
service=""
tag=""
env=""
sdir=""

while getopts "s:t:e:d:" opt; do
    case $opt in
    s)
        service=$OPTARG
        ;;
    t)
        tag=$OPTARG
        ;;
    e)
        env=$OPTARG
        ;;
    d)
        sdir=$OPTARG
        ;;
    *)
        Usage
        ;;
    esac
done
if [ "$service" == "" ]; then
    echo "missing service value"
    Usage
fi
if [ "$tag" == "" ]; then
    echo "missing tag value"
    Usage
fi
if [ "$env" == "" ]; then
    echo "missing env value"
    Usage
fi
echo "service=$service tag=$tag env=$env sdir=$sdir"

If you'll run it you'll see something like this:

# the case if we miss mandatory input parameter
./t.sh -e test -s dbs
missing tag value
t.sh -s <service> -t <tag> -e <env> -d <optional services directory>

# the case when all mandatory parameters are in place
./t.sh -e test -s dbs -t HG123
service=dbs tag=HG123 env=test sdir=

@muhammadimranfarooqi , @arooshap please review and test these changes by yourself in your own environment and report if everything is fine and you are satisfied with them (i.e. I mean try your wokrflows and test with multiple setups).

@amaltaro please provide your feedback as user if you are satisfied with proposed solution.

@arooshap
Copy link
Member

Hi @vkuznet,

I have tested this script separately on many instances, and it works as expected.

@arooshap
Copy link
Member

@vkuznet @amaltaro would you like to merge this PR or keep it open?

@vkuznet
Copy link
Collaborator Author

vkuznet commented Jun 16, 2023

@arooshap , I'm in favor of merging this PR but I will let Alan to confirm if it is ready or will require further adjustments.

@arooshap arooshap requested a review from amaltaro October 6, 2023 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants