[RFC,1/2] pw_mon: improve command line options

  In the future, we'll use this to add support for passing opts into some parts
of pw_mon.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 pw_mon | 53 +++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 14 deletions(-)
  

On Fri, Oct 27, 2023 at 9:06 AM Aaron Conole <aconole@redhat.com> wrote:
>
> In the future, we'll use this to add support for passing opts into some parts
> of pw_mon.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  pw_mon | 53 +++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/pw_mon b/pw_mon
> index 28feb8b..01bdd25 100755
> --- a/pw_mon
> +++ b/pw_mon
> @@ -21,34 +21,59 @@
>
>  [ -f "${HOME}/.pwmon-rc" ] && source "${HOME}/.pwmon-rc"
>
> -if [ "$1" != "" ]; then
> -    pw_project="$1"
> -    shift
> +if [ "$1" != ""  ]; then
nitpick: there is an extra white space between "" and ]
> +    if ! echo "$1" | grep -E ^-- >/dev/null 2>&1; then
I am trying to understand what you are trying to accomplish with these
if statements and the while loop. Couldnt you just move everything
into the while loop so you dont have to repeat the checks? I mean, you
will probably still have to use an OR || to check both conditions,
with and without the "--" at the begining. Maybe you could use some
sort of regex to make the "--" optional and then maybe you could
combine both checks into a single check and make it much easier

But just to double check, you are trying to parse both
"--pw-instance=myinstance" and "pw-instance=myinstance". Correct?
That's the whole goal of this patch?

The lack of strings around ^-- make me really unseasy, this also
applies to the sed line. Also, with grep you can tell it to not dump
any output/error. see -s and -q options. These apply to the other grep
commands in the script as well

> +        pw_project="$1"
> +        shift
> +    fi
>  fi
>
>  if [ "$1" != "" ]; then
> -    pw_instance="$1"
> -    shift
> -fi
> -
> -if [ "X$pw_instance" == "X" -o "X$pw_project" == "X" ]; then
> -   echo "ERROR: Patchwork instance and project are unset."
> -   echo "Please setup ${HOME}/.pwmon-rc and set pw_project "
> -   echo "(or pass it as an argument)."
> -   echo "Also either setup pw_instance or pass it as an argument."
> -   exit 1
> +    if ! echo "$1" | grep -E ^-- >/dev/null 2>&1; then
> +        pw_instance="$1"
> +        shift
> +    fi
>  fi
>
>  userpw=""
>
>  if [ "$1" != "" ]; then
> -    pw_credential="$1"
> +    if ! echo "$1" | grep -E ^-- >/dev/null 2>&1; then
why is this not inside the while loop? the use of -- forbidden for
credentials via the cli?
> +        pw_credential="$1"
> +        shift
> +    fi
>  fi
>
> +
> +while [ "$1" != "" ]; do
Im guessing the whole point of this patch was to add this while loop
> +    if echo "$1" | grep -E ^--pw-project= >/dev/null 2>&1; then
> +        pw_project=$(echo "$1" | sed s/--pw-project=//)
> +        shift
> +    elif echo "$1" | grep -E ^--pw-instance= >/dev/null 2>&1; then
> +        pw_instance=$(echo "$1" | sed s/--pw-instance=//)
> +        shift
> +    elif echo "$1" | grep -E ^--help >/dev/null 2>&1; then
> +        echo "pw_mon script"
> +        echo "$0 [proj|--pw-project=<proj>] [instance|--pw-instance=<inst url] opts.."
> +        echo "Options:"
> +        echo "    --add-filter-recheck=filter  Adds a filter to flag that a recheck needs to be done"
> +        echo ""
> +        exit 0
> +    fi
> +done
> +
>  if [ "X$pw_credential" != "X" ]; then
>     userpw="-u \"${pw_credential}\""
>  fi
>
> +if [ "X$pw_instance" == "X" -o "X$pw_project" == "X" ]; then
> +   echo "ERROR: Patchwork instance and project are unset."
> +   echo "Please setup ${HOME}/.pwmon-rc and set pw_project "
> +   echo "(or pass it as an argument)."
> +   echo "Also either setup pw_instance or pass it as an argument."
> +   exit 1
> +fi
> +
>  source $(dirname $0)/series_db_lib.sh
>
>  function emit_series() {
> --
> 2.41.0
>
  

Michael Santana <msantana@redhat.com> writes:

> On Fri, Oct 27, 2023 at 9:06 AM Aaron Conole <aconole@redhat.com> wrote:
>>
>> In the future, we'll use this to add support for passing opts into some parts
>> of pw_mon.
>>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  pw_mon | 53 +++++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 39 insertions(+), 14 deletions(-)
>>
>> diff --git a/pw_mon b/pw_mon
>> index 28feb8b..01bdd25 100755
>> --- a/pw_mon
>> +++ b/pw_mon
>> @@ -21,34 +21,59 @@
>>
>>  [ -f "${HOME}/.pwmon-rc" ] && source "${HOME}/.pwmon-rc"
>>
>> -if [ "$1" != "" ]; then
>> -    pw_project="$1"
>> -    shift
>> +if [ "$1" != ""  ]; then
> nitpick: there is an extra white space between "" and ]
>> +    if ! echo "$1" | grep -E ^-- >/dev/null 2>&1; then

> I am trying to understand what you are trying to accomplish with these
> if statements and the while loop. Couldnt you just move everything
> into the while loop so you dont have to repeat the checks? I mean, you
> will probably still have to use an OR || to check both conditions,
> with and without the "--" at the begining. Maybe you could use some
> sort of regex to make the "--" optional and then maybe you could
> combine both checks into a single check and make it much easier

Not exactly.  We need to keep some compatibility, so if the first
argument doesn't start with "--" then we will treat it like a legacy
parameter.

> But just to double check, you are trying to parse both
> "--pw-instance=myinstance" and "pw-instance=myinstance". Correct?
> That's the whole goal of this patch?

No.  Original functionality would be like:

  $ ./pw_mon project instance credentials

New functionality accepts this, and also will accept:

  $ ./pw_mon --pw-project=project --pw-instance=instance ...

> The lack of strings around ^-- make me really unseasy, this also
> applies to the sed line. 

Why?  You think it needs to be escaped?

> Also, with grep you can tell it to not dump
> any output/error. see -s and -q options. These apply to the other grep
> commands in the script as well

Okay - I'll use -q and -s instead of redirections.

>> +        pw_project="$1"
>> +        shift
>> +    fi
>>  fi
>>
>>  if [ "$1" != "" ]; then
>> -    pw_instance="$1"
>> -    shift
>> -fi
>> -
>> -if [ "X$pw_instance" == "X" -o "X$pw_project" == "X" ]; then
>> -   echo "ERROR: Patchwork instance and project are unset."
>> -   echo "Please setup ${HOME}/.pwmon-rc and set pw_project "
>> -   echo "(or pass it as an argument)."
>> -   echo "Also either setup pw_instance or pass it as an argument."
>> -   exit 1
>> +    if ! echo "$1" | grep -E ^-- >/dev/null 2>&1; then
>> +        pw_instance="$1"
>> +        shift
>> +    fi
>>  fi
>>
>>  userpw=""
>>
>>  if [ "$1" != "" ]; then
>> -    pw_credential="$1"
>> +    if ! echo "$1" | grep -E ^-- >/dev/null 2>&1; then

> why is this not inside the while loop? the use of -- forbidden for
> credentials via the cli?

No - ^-- means only match -- at the beginning of the line.

But there's a good catch here that we don't actually preserve the
ability to set pw_credentials via a new style option.

>> +        pw_credential="$1"
>> +        shift
>> +    fi
>>  fi
>>
>> +
>> +while [ "$1" != "" ]; do
> Im guessing the whole point of this patch was to add this while loop
>> +    if echo "$1" | grep -E ^--pw-project= >/dev/null 2>&1; then
>> +        pw_project=$(echo "$1" | sed s/--pw-project=//)
>> +        shift
>> +    elif echo "$1" | grep -E ^--pw-instance= >/dev/null 2>&1; then
>> +        pw_instance=$(echo "$1" | sed s/--pw-instance=//)
>> +        shift
>> +    elif echo "$1" | grep -E ^--help >/dev/null 2>&1; then
>> +        echo "pw_mon script"
>> +        echo "$0 [proj|--pw-project=<proj>] [instance|--pw-instance=<inst url] opts.."
>> +        echo "Options:"
>> +        echo "    --add-filter-recheck=filter  Adds a filter to flag that a recheck needs to be done"
>> +        echo ""
>> +        exit 0
>> +    fi
>> +done
>> +
>>  if [ "X$pw_credential" != "X" ]; then
>>     userpw="-u \"${pw_credential}\""
>>  fi
>>
>> +if [ "X$pw_instance" == "X" -o "X$pw_project" == "X" ]; then
>> +   echo "ERROR: Patchwork instance and project are unset."
>> +   echo "Please setup ${HOME}/.pwmon-rc and set pw_project "
>> +   echo "(or pass it as an argument)."
>> +   echo "Also either setup pw_instance or pass it as an argument."
>> +   exit 1
>> +fi
>> +
>>  source $(dirname $0)/series_db_lib.sh
>>
>>  function emit_series() {
>> --
>> 2.41.0
>>