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

onedir install - correct version regex #1868

Merged
merged 14 commits into from
Sep 16, 2022

Conversation

jeff350
Copy link
Contributor

@jeff350 jeff350 commented Sep 13, 2022

What does this PR do?

This PR fixes an issue with the regex where it was looking for versions in format 3005-1 when it should be looking for 3005.1

This is adjusting the regex to the same as the stable install.

This also fixes the ability to call the function with 3005.0 and have it resolve to 3005 as is done in the stable install.

@garethgreenaway
Copy link
Contributor

garethgreenaway commented Sep 13, 2022

@jeff350 Thanks for the PR. The bootstrap needs to handle both scenarios, the one you've updated it to handle and the one it was previously handling. Our minor versions are now under minor directory, eg. https://repo.saltproject.io/salt/py3/redhat/9/x86_64/minor/3005-1/ and the major versions are here, eg. https://repo.saltproject.io/salt/py3/redhat/9/x86_64/3005/. To handle both situations we need something like this:

        if [ "$(echo "$1" | grep -E '^(latest)$')" != "" ]; then
            ONEDIR_REV="$1"
            shift
        elif [ "$(echo "$1" | grep -E '^([3-9][0-9]{3}?-[0-9]$)')" != "" ]; then
            ONEDIR_REV="minor/$1"
            shift
        elif [ "$(echo "$1" | grep -E '^([3-9][0-9]{3}(\.[0-9]*)?)')" != "" ]; then
            # Handle the 3xxx.0 version as 3xxx archive (pin to minor) and strip the fake ".0" suffix
            ONEDIR_REV=$(echo "$1" | sed -E 's/^([3-9][0-9]{3})\.0$/\1/')
            shift

@jeff350
Copy link
Contributor Author

jeff350 commented Sep 14, 2022

@garethgreenaway Thank you for the explanation of what needs to be supported. I got the requested changes added in and all tests are passing.

looking at this again does the 3rd section handling 3005.0 format need to be changed to

        elif [ "$(echo "$1" | grep -E '^([3-9][0-9]{3}(\.0)?)$')" != "" ]; then
            # Handle the 3xxx.0 version as 3xxx archive (pin to minor) and strip the fake ".0" suffix
            ONEDIR_REV=$(echo "$1" | sed -E 's/^([3-9][0-9]{3})\.0$/\1/')
            shift

so we do not end up with ONEDIR_REV being set to 3005.1?

@garethgreenaway
Copy link
Contributor

@jeff350 Apologies for the delay, I chatted with a few other members of the Salt project core team about our plans for the repo layout and it should be pretty close to what we have for the classic non-onedir packages. The major different is the use of minor instead of archive. The following block should accomplish what we want, not the addition of being able to specific a major version of 3005 along with latest.

elif [ "$ITYPE" = "onedir" ]; then
    if [ "$#" -eq 0 ];then
        ONEDIR_REV="latest"
    else
        if [ "$(echo "$1" | grep -E '^(latest|3005)$')" != "" ]; then
            ONEDIR_REV="$1"
            shift
        elif [ "$(echo "$1" | grep -E '^([3-9][0-9]{3}(\.[0-9]*)?)')" != "" ]; then
            # Handle the 3xxx.0 version as 3xxx archive (pin to minor) and strip the fake ".0" suffix
            ONEDIR_REV=$(echo "$1" | sed -E 's/^([3-9][0-9]{3})\.0$/\1/')
            ONEDIR_REV="minor/$ONEDIR_REV"
            shift
        else
            echo "Unknown stable version: $1 (valid: 3005, latest.)"
            exit 1
        fi
    fi

@jeff350
Copy link
Contributor Author

jeff350 commented Sep 15, 2022

@garethgreenaway I appreciate you taking the time to talk this over with others on the team to get this figured out. unfortunately I am not following.

Earlier you stated that this needed to handle both scenarios of MAJOR.MINOR and MAJOR-MINOR. The code you posted does not account for the MAJOR-MINOR format. Could you please clarify if both of these need to be handled?

My understanding is that the following is what is required.

If we are handling the following input and output formats
INPUTS | ONEDIR_REV formats

  • 'latest' -> "latest"
  • '3005' -> "3005"
  • MAJOR.MINOR -> "minor/MAJOR.MINOR"
  • MAJOR-MINOR -> "minor/MAJOR-MINOR"
  • MAJOR.0 -> "minor/MAJOR"

we would want this block.

elif [ "$ITYPE" = "onedir" ]; then
    if [ "$#" -eq 0 ];then
        ONEDIR_REV="latest"
    else
        if [ "$(echo "$1" | grep -E '^(latest|3005)$')" != "" ]; then
            ONEDIR_REV="$1"
            shift
        elif [ "$(echo "$1" | grep -E '^([3-9][0-9]{3}((\.|\-)[0-9]*)?)$')" != "" ]; then
            # Handle the 3xxx.0 version as 3xxx archive (pin to minor) and strip the fake ".0" suffix
            ONEDIR_REV=$(echo "$1" | sed -E 's/^([3-9][0-9]{3})\.0$/\1/')
            ONEDIR_REV="minor/$ONEDIR_REV"
            shift
        else
            echo "Unknown stable version: $1 (valid: 3005, latest.)"
            exit 1
        fi
    fi

The other questions that come to mind are:

  1. Do we want to handle the format MAJOR-0 and strip off the fake "-0"?
  2. Do we want to have 3005 directly in the grep or use [3-9][0-9]{3} to prevent the need for updating the script for every major salt release?

@garethgreenaway
Copy link
Contributor

@jeff350 When I mentioned needing to support the MAJOR-MINOR that was a mistake and left over from additions to the bootstrap script to install the RC (release candidate) packages, the number following the - is going to the package number, eg. when we build a new package that number increments. With the move towards the onedir packages there will likely be a case when we rebuild the package but the Salt code for that package has not changed, for example a security release for Python.

So the scenarios that need to be supported are:

  • 'latest' -> "latest"
    
  • '3005' -> "3005"
    
  • MAJOR.MINOR -> "minor/MAJOR.MINOR"
    
  • MAJOR.0 -> "minor/MAJOR"
    
1.     Do we want to handle the format MAJOR-0 and strip off the fake "-0"?

No. We won't have a MAJOR-0 . When the 3006 release is released we'll be moving back to using .0 for releases, so the first one out will be 3006.0 and the package will be 3006.0-1 but the repo layout will have a 3006.0 location. This will require changes to not strip the trailing 0 for that release.

2.     Do we want to have 3005 directly in the grep or use [3-9][0-9]{3} to prevent the need for updating the script for every major salt release?

I think for now sticking to the having the version number in there as it's a check for a valid release.

@garethgreenaway garethgreenaway merged commit 614fac2 into saltstack:develop Sep 16, 2022
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.

3 participants