Fix error message for older template version registration

[Copilot]

When trying to register an older bundle version you get a 409 "A template with this version already exists" error instead of a more accurate message like "A newer version of this template already exists. (existing: 1.2.6, attempted: 1.2.1) Registration aborted." the error should actually come from register_bundle_with_api.sh instead of the API.

So if you (incorrectly try to register an older version of an existing template it should fail before sending the register api request.

To Fix:
The logic in function get_template devops/scripts/register_bundle_with_api.sh should check if the version is the same or older and exit with code 1 and a useful error so it errors at that stage instead of continuing the request.
The message for the 409 error could also be updated to reflect that the version could differ.

The existing code only checks equal versions but not less than.
`if [[ "$(echo "$get_result" | jq -r .version)" == "$template_version" ]]; then

Fixes #4685

[marrobi]

@copilot you haven't committed anything.

[marrobi]

@copilot nothing is commited.

[JC-wk]

@marrobi I noticed I could assign copilot so I tried it out but I don't yet have a license for it so it didn't do anything. I think you will need to reassign copilot to #4685 and see if it picks this branch up or starts a new one.

[github-actions]

Unit Test Results

658 tests   658 ✅  6s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit a5c273a.

♻️ This comment has been updated with latest results.

[JC-wk]

This PR adds a semantic version comparison to prevent attempts to upload an older version of a bundle when a newer one exists and display a more relevant error message

[JC-wk]

You can test the semver comparison with the below script.

#!/bin/bash

semver_compare() {
  local -a new_ver cur_ver
  IFS='.' read -r -a new_ver <<< "$1"
  IFS='.' read -r -a cur_ver <<< "$2"

  # Compare up to the max length; missing parts are treated as 0
  local i max=${#new_ver[@]}
  (( ${#cur_ver[@]} > max )) && max=${#cur_ver[@]}

  for ((i=0; i<max; i++)); do
    # default empty parts to 0; force base-10 to avoid octal interpretation
    local a=${new_ver[i]:-0}
    local b=${cur_ver[i]:-0}
    if ((10#$a < 10#$b)); then return 0; fi # new ver < current ver (error)
    if ((10#$a > 10#$b)); then return 2; fi # new ver > current ver (ok to register)
  done
  return 1  # new ver == current ver (ignore)
}

test() { semver_compare "$1" "$2"; echo "$1 ? $2  -> $?" ; }

test 1.2.3 1.2.4      # expect 0 (older) new ver < current ver (error)
test 1.2.3 1.2.3      # expect 1 (equal)
test 1.2.4 1.2.3      # expect 2 (newer) new ver > current ver (ok to register)
test 1.2   1.2.0      # expect 1 (equal)
test 1.2.0 1.10.0     # expect 0 (older) new ver < current ver (error)
test 1.10.0 1.2.0     # expect 2 (newer) new ver > current ver (ok to register)

  ver_lt() { semver_compare "$1" "$2"; return $(( $? == 0 ? 0 : 1 )); }
  ver_eq() { semver_compare "$1" "$2"; return $(( $? == 1 ? 0 : 1 )); }

  template_version="0.7.7"
  existing_version="0.7.7"

  if ver_eq "$template_version" "$existing_version"; then
    echo "A template with this version already exists."
    exit 0
  elif ver_lt "$template_version" "$existing_version"; then
    echo "A newer version already exists (existing: $existing_version, attempted: $template_version). Registration aborted."
    exit 1
  fi

[martinpeck]

I'm a little concerned about putting complex code into bash scripts. It feels like this bug (which, as mentioned in the issue, is low priority) could be addressed by changing the text from...

"Template with this version already exists"

...to...

"Template with this version, or newer, already exists"

(or some other message that better explains the reasons why this failure might have happened).

... rather than adding this semver code into the script. My suggestion is that we either close the issue as "won't fix" or take the easier-to-maintain path of simply updating the error message.

[JC-wk]

@martinpeck the problem is that the existing behaviour of the current comparison does not work for less than because it is only checking for equality so it attempts to push older versions and then the api doesn't give an easily understandable error, there are other ways to achieve the semver comparison but they rely on tools being available on the OS.

[martinpeck]

Ah, right...so it's the API that's giving the wrong/unclear error?

So, if the API returned a better error ("Template with this version, or newer, already exists") would that also fix things?

[JC-wk]

Yes, the API returns 409 Conflict: "A template with this version already exists" however when versions are equal the API request is never made, so it's also inconsistent behaviour between the two scenarios.

[marrobi]

Maybe just best to rely on the API?

[JC-wk]

If the error message from the API was more accurate then that would be fine

[marrobi]

I suggest we do that. Maybe ask copilot.

[JC-wk]

since we run in a devcontainer there are one liners in linux like the sort command that would remove the bash complexity, I can give those a try and see and also look at the api

[JC-wk]

Hi @martinpeck @marrobi
I have simplified this to use the following one liner instead of the bash function
[[ "$(printf '%s\n' "$template_version" "$existing_version" | sort -V | head -n1)" != "$existing_version" ]]
Is that okay?
I will also update the api text

[marrobi]

LGTM

[marrobi]

/test 68af3f0

[github-actions]

🤖 pr-bot 🤖

⚠️ The specified SHA 68af3f0 is not the latest commit on the PR. Please validate the latest commit and re-run /test

(in response to this comment from @marrobi)