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

#1112: Version-prefix not working as expected #1137

Merged

Conversation

Leosssss
Copy link
Contributor

@Leosssss Leosssss commented May 3, 2023

bugfix: add a condition to check if the version_prefix end with a digit

@github-actions github-actions bot added bash related to bash shell or scripts scripts related to shell scripts (bash and CMD) labels May 3, 2023
@Leosssss
Copy link
Contributor Author

Leosssss commented May 3, 2023

This PR is a bugfix PR, so we don't need to document this in the CHANGELOG?

Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

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

@Leosssss thanks for your PR. You figured out correct solutions to determine if the version_prefix ends with a digit and also how to escape a . to prevent it from matching any character. Great work that already works and solves the root problem described in the story 👍

Sorry, for being picky but could you do the escaping in both cases (I could still do JAVA_VERSION=17.0* and even with your fix, this would also match a potential version 1720.0 - theoretical case but lets get this fixed once and forever)? And could you also avoid the redundancy by using a local variable that is either empty or [^0-9] so you can reuse the same code without duplication?

@Leosssss
Copy link
Contributor Author

@hohwille Thank you for your feedback, I still have a question, what should happen when JAVA_VERSION=17.0*? Should the regex return no results?

Leosssss and others added 4 commits May 12, 2023 13:06
@Leosssss Leosssss marked this pull request as draft May 17, 2023 13:55
@Leosssss Leosssss marked this pull request as ready for review May 17, 2023 13:55
@CREITZ25 CREITZ25 linked an issue May 17, 2023 that may be closed by this pull request
@CREITZ25 CREITZ25 marked this pull request as draft May 17, 2023 13:58
@CREITZ25 CREITZ25 marked this pull request as ready for review May 17, 2023 13:59
@hohwille
Copy link
Member

To demonstrate my point, I have copied your code into this script:

#!/bin/bash

function version_prefix() {
  local version_prefix=$1
  local available_versions="${DEVON_IDE_HOME}/mirrors/java/available-versions"
  local resolved_version
  local regex="[0-9]*\."
  if [[ "${version_prefix}" =~ $regex ]]
  then
    version_prefix="${version_prefix/./\\.}"
  fi
  resolved_version="$(grep "^${version_prefix}" "${available_versions}" | head -n 1)"
  echo $resolved_version
}

version_prefix $@

I added the versions 17.1.0 and 17.10.0 to the available versions locally and get this:

./version-prefix.sh 17.0*
17.10.0

This is my final understanding of this issue: version-prefix is used to find the latest version STARTING with version-prefix. It should be noted that when e.g. version-prefix is "1.1*", only version numbers starting with "1.1.xxx" can be matched, while versions like "1.10.xxx" should not be matched. The version "1.10.xxx" is not the version that can be matched by "1.1*". To match "1.10.xxx", the version_prefix should be "1.10*"/"1.10.*"
@hohwille
Copy link
Member

For the build error, see the log:

Running test #4: test-shellcheck

In scripts/functions line 1288:
      version_prefix="$(echo "${version_prefix::-1}" | sed 's/[[:punct:]]/[.]/g')"
                        ^-- SC2001 (style): See if you can use ${variable//search/replace} instead.

For more information:
  https://www.shellcheck.net/wiki/SC2001 -- See if you can use ${variable//se...
[ERROR] Failed running test #4: test-shellcheck - exit code 1

Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

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

@Leosssss thanks for the rework. This PR fixes only one aspect of two from #1112.
However, I will approve and merge but keep #1112 open until also the second aspect ([^0-9] only if version_prefix ends with digit) is fixed what was the actual problem described in the story.

@hohwille hohwille changed the title #1112 version prefix not working as expected #1112: dot in version_prefix shall only match dot but not any other character May 30, 2023
Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

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

@Leosssss thanks for your rework. 👍 You got very close but after looking deeper, I still found some problems. I will better do a constructive review and change the code accordingly and merge it. I hope this was a good learning on how fiddly the bash syntax is and how important testing with the different given examples is in IT business.

scripts/src/main/resources/scripts/functions Outdated Show resolved Hide resolved
scripts/src/main/resources/scripts/functions Outdated Show resolved Hide resolved
@hohwille hohwille changed the title #1112: dot in version_prefix shall only match dot but not any other character #1112: Version-prefix not working as expected Jun 2, 2023
@hohwille hohwille added this to the release:2023.05.001 milestone Jun 2, 2023
@hohwille hohwille merged commit d396136 into devonfw:master Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bash related to bash shell or scripts scripts related to shell scripts (bash and CMD)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version-prefix not working as expected
2 participants