core: make alternate image flag unpositioned for debug mode

[parth-gr]

Closes: #44

Signed-off-by: parth-gr paarora@redhat.com

[subhamkrai]

@parth-gr could you use vsCode extention like shell-format shellcheck for bash formatting? I see space and other lint.

[subhamkrai]

Also, I'll let @BlaineEXE review the flags changes.

[BlaineEXE]

What about this? I'm finding the if constructs hard to read, and I'm not sure the code seems like it will work as intended. This also ensures with end_of_command_parsing that there aren't extraneous arguments also.

DEBUG_DEPLOYMENT_NAME=""

function parse_extra_start_debug_flags() {
  [[ -z "${1:-""}" ]] && fail_error "Missing mon or osd deployment name"
  DEBUG_DEPLOYMENT_NAME="$1"
  parse_flags parse_image_flag "${REMAINING_ARGS[@]}"
  end_of_command_parsing "${REMAINING_ARGS[@]}" # end of command tree
}

function run_start_debug() {
  parse_flags parse_image_flag "$@"
  parse_extra_start_debug_flags "${REMAINING_ARGS[@]}"
  
  [[ -z "${DEBUG_DEPLOYMENT_NAME:-""}" ]] && fail_error "Failed to parse mon or osd deployment name"
  
  # ... etc. ...
}

[parth-gr]

@BlaineEXE

The current code works completely fine

[rider@localhost kubectl-rook-ceph]$ ./kubectl-rook-ceph.sh debug start --alternate-image imagename deploymentName 
Deployment Name: deploymentName
Alternate Image Name:imagename
[rider@localhost kubectl-rook-ceph]$ 
[rider@localhost kubectl-rook-ceph]$ ./kubectl-rook-ceph.sh debug start deploymentName --alternate-image imagename
Deployment Name: deploymentName
Alternate Image Name:imagename
[rider@localhost kubectl-rook-ceph]$ 
[rider@localhost kubectl-rook-ceph]$ 
[rider@localhost kubectl-rook-ceph]$ ./kubectl-rook-ceph.sh debug start deploymentName
Deployment Name: deploymentName
Alternate Image Name:
[rider@localhost kubectl-rook-ceph]$ 
[rider@localhost kubectl-rook-ceph]$ ./kubectl-rook-ceph.sh debug start --alternate-image imagename
Error:  Missing mon or osd deployment name
[rider@localhost kubectl-rook-ceph]$ ./kubectl-rook-ceph.sh debug start deploymentName --alternate-image
./kubectl-rook-ceph.sh: line 136: $1: unbound variable

[parth-gr]

The above code and end_of_command_parsing cannot be used, the command needs to be shifted when deployment name is at starting as the "${REMAINING_ARGS[@]}" will still contain the deployment name, so it need array shifting and all
And also for end_of_command_parsing we need shift if the deployment name is at the last as the "${REMAINING_ARGS[@]} will still contain the deployment name

And also it needs another Debug_deploymernt global variable to make use

If you just want to shift this code into a function I can do it.

[BlaineEXE]

We should be using end_of_command_parsing to flag issues where users give too many commands.

[BlaineEXE]

Also, the script should never error because of an unbound variable.

Even if this code works, I am having trouble reading the code's intent, which usually means maintaining the code in the future will be more difficult. I think breaking it into a function and treating DEPLOYMENT_NAME as an argument (which it is) will make things more clear. And on that note as well, comments to explain what is happening will help too.

[BlaineEXE]

I played with this a bit and found that using a function does seem confusing. I have a further suggestion to help keep the code readable. It doesn't rely on conditionals of side-effects, so it is more straightforward to understand, and the comments should help as well. Plus, it uses end_of_command_parsing to accurately determine if a user has supplied too many commands.

ALTERNATE_IMAGE=""

function run_start_debug(){
  parse_flags parse_image_flag "$@" # parse flags before the deployment name
  deployment_name="${REMAINING_ARGS[0]}" # get deployment name
  REMAINING_ARGS=("${REMAINING_ARGS[@]:1}") # remove deploy name from remaining args
  parse_flags parse_image_flag "${REMAINING_ARGS[@]}" # parse flags after the deployment name
  end_of_command_parsing "${REMAINING_ARGS[@]}"

  verify_debug_deployment "$deployment_name"

  # copy the deployment spec before scaling it down
  deployment_spec=$(KUBECTL_NS_CLUSTER get deployments "$deployment_name" -o json | jq -r ".spec")
  # ... etc. ...

This will also make it easier to add new flags in the future. Using conditionals on the assumption that there is only one flag will break if we add another flag to make this tool more configurable in the future.

[BlaineEXE]

I don't see the changes I mentioned just above

[parth-gr]

Sorry, I missed this comment,
addressed it

[BlaineEXE]

I'm not sure if this is what you intend here, but REMAINING_ARGS is an array. "${REMAINING_ARGS}" will implicitly return only the first item of the array. Generally, implicit behavior makes code harder to read, and I suggest using "${REMAINING_ARGS[0]:-""}" to make it more explicit that the intent is to check only the first array item.

[parth-gr]

Done

[subhamkrai]

approving my changes has been addressed

[BlaineEXE]

I don't see the changes I mentioned just above