-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix(bootstrap.sh): bump the mariadb version from 11.1 to 11.2 #12
Conversation
+ 11.1 no longer exists on the mirror server we're using, which causes the bootstrap.sh script to fail (on the 404), which causes the `./mwaa-local-env start` command to fail
WalkthroughThe pull request modifies the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docker/script/bootstrap.sh (2)
56-62
: Consider quoting$(uname -p)
to prevent word splittingWhile the current implementation works, it's best practice to quote command substitutions to prevent word splitting and potential issues with special characters.
Apply this change to all affected lines:
- wget https://mirror.mariadb.org/yum/11.2/fedora38-aarch64/rpms/MariaDB-common-11.2.2-1.fc38.$(uname -p).rpm -P /mariadb_rpm + wget https://mirror.mariadb.org/yum/11.2/fedora38-aarch64/rpms/MariaDB-common-11.2.2-1.fc38."$(uname -p)".rpm -P /mariadb_rpm🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 56-56: Quote this to prevent word splitting.
(SC2046)
[warning] 57-57: Quote this to prevent word splitting.
(SC2046)
[warning] 58-58: Quote this to prevent word splitting.
(SC2046)
[warning] 60-60: Quote this to prevent word splitting.
(SC2046)
[warning] 61-61: Quote this to prevent word splitting.
(SC2046)
[warning] 62-62: Quote this to prevent word splitting.
(SC2046)
56-62
: Consider using variables for version numbersTo make future version updates easier to manage, consider extracting the version number into a variable at the top of the script.
#!/bin/sh set -e set -x + MARIADB_VERSION="11.2" + MARIADB_RELEASE="11.2.2-1.fc38" # ... rest of the script ... if [[ $(uname -p) == "aarch64" ]]; then - wget https://mirror.mariadb.org/yum/11.2/fedora38-aarch64/rpms/MariaDB-common-11.2.2-1.fc38.$(uname -p).rpm -P /mariadb_rpm + wget "https://mirror.mariadb.org/yum/${MARIADB_VERSION}/fedora38-aarch64/rpms/MariaDB-common-${MARIADB_RELEASE}.$(uname -p).rpm" -P /mariadb_rpm🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 56-56: Quote this to prevent word splitting.
(SC2046)
[warning] 57-57: Quote this to prevent word splitting.
(SC2046)
[warning] 58-58: Quote this to prevent word splitting.
(SC2046)
[warning] 60-60: Quote this to prevent word splitting.
(SC2046)
[warning] 61-61: Quote this to prevent word splitting.
(SC2046)
[warning] 62-62: Quote this to prevent word splitting.
(SC2046)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docker/script/bootstrap.sh
(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
docker/script/bootstrap.sh
[warning] 56-56: Quote this to prevent word splitting.
(SC2046)
[warning] 57-57: Quote this to prevent word splitting.
(SC2046)
[warning] 58-58: Quote this to prevent word splitting.
(SC2046)
[warning] 60-60: Quote this to prevent word splitting.
(SC2046)
[warning] 61-61: Quote this to prevent word splitting.
(SC2046)
[warning] 62-62: Quote this to prevent word splitting.
(SC2046)
wget https://mirror.mariadb.org/yum/11.2/fedora38-aarch64/rpms/MariaDB-common-11.2.2-1.fc38.$(uname -p).rpm -P /mariadb_rpm | ||
wget https://mirror.mariadb.org/yum/11.2/fedora38-aarch64/rpms/MariaDB-shared-11.2.2-1.fc38.$(uname -p).rpm -P /mariadb_rpm | ||
wget https://mirror.mariadb.org/yum/11.2/fedora38-aarch64/rpms/MariaDB-devel-11.2.2-1.fc38.$(uname -p).rpm -P /mariadb_rpm | ||
else | ||
wget https://mirror.mariadb.org/yum/11.1/fedora38-amd64/rpms/MariaDB-common-11.1.2-1.fc38.$(uname -p).rpm -P /mariadb_rpm | ||
wget https://mirror.mariadb.org/yum/11.1/fedora38-amd64/rpms/MariaDB-shared-11.1.2-1.fc38.$(uname -p).rpm -P /mariadb_rpm | ||
wget https://mirror.mariadb.org/yum/11.1/fedora38-amd64/rpms/MariaDB-devel-11.1.2-1.fc38.$(uname -p).rpm -P /mariadb_rpm | ||
wget https://mirror.mariadb.org/yum/11.2/fedora38-amd64/rpms/MariaDB-common-11.2.2-1.fc38.$(uname -p).rpm -P /mariadb_rpm | ||
wget https://mirror.mariadb.org/yum/11.2/fedora38-amd64/rpms/MariaDB-shared-11.2.2-1.fc38.$(uname -p).rpm -P /mariadb_rpm | ||
wget https://mirror.mariadb.org/yum/11.2/fedora38-amd64/rpms/MariaDB-devel-11.2.2-1.fc38.$(uname -p).rpm -P /mariadb_rpm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Consider upgrading to MariaDB 11.6.1 for better long-term maintenance
The verification shows that version 11.2.2 packages are being redirected (302 Found), while 11.6.1 is the latest available version in both architectures with direct downloads. For better maintainability, consider updating to version 11.6.1:
- Replace
11.2.2
with11.6.1
in the package URLs - Both aarch64 and amd64 architectures have consistent package availability
- Version 11.7 directory exists but doesn't contain the required packages yet
Example update for aarch64:
https://mirror.mariadb.org/yum/11.6/fedora38-aarch64/rpms/MariaDB-common-11.6.1-1.fc38.$(uname -p).rpm
🔗 Analysis chain
Verify MariaDB version selection and availability
The PR mentions version 11.7 as a potential alternative. Let's verify the availability and stability of different versions.
While version 11.2.2 resolves the immediate 404 error, we should confirm if using the latest stable version (11.7) would be more beneficial for long-term maintenance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check available MariaDB versions and their availability on the mirror
# Test both architectures to ensure consistent availability
echo "Checking aarch64 versions..."
curl -sI "https://mirror.mariadb.org/yum/11.2/fedora38-aarch64/rpms/" | grep "HTTP/"
curl -sI "https://mirror.mariadb.org/yum/11.7/fedora38-aarch64/rpms/" | grep "HTTP/"
echo "Checking amd64 versions..."
curl -sI "https://mirror.mariadb.org/yum/11.2/fedora38-amd64/rpms/" | grep "HTTP/"
curl -sI "https://mirror.mariadb.org/yum/11.7/fedora38-amd64/rpms/" | grep "HTTP/"
# Check if these are the latest stable versions
curl -s "https://downloads.mariadb.org/rest-api/mariadb/" | jq -r '.[] | select(.release_status=="stable") | .release_id'
Length of output: 721
Script:
#!/bin/bash
# Let's check the actual package availability and versions for both architectures
echo "Checking aarch64 packages..."
curl -s "https://mirror.mariadb.org/yum/11.7/fedora38-aarch64/rpms/" | grep -o 'MariaDB-\(common\|shared\|devel\)-[0-9.]\+-[0-9]\.fc38\.aarch64\.rpm' || true
echo -e "\nChecking amd64 packages..."
curl -s "https://mirror.mariadb.org/yum/11.7/fedora38-amd64/rpms/" | grep -o 'MariaDB-\(common\|shared\|devel\)-[0-9.]\+-[0-9]\.fc38\.x86_64\.rpm' || true
# Let's also check if the current 11.2.2 packages are still available
echo -e "\nVerifying current 11.2.2 packages..."
for arch in "aarch64" "amd64"; do
for pkg in "common" "shared" "devel"; do
curl -sI "https://mirror.mariadb.org/yum/11.2/fedora38-${arch}/rpms/MariaDB-${pkg}-11.2.2-1.fc38.$([ "$arch" = "amd64" ] && echo "x86_64" || echo "aarch64").rpm" | grep "HTTP/"
done
done
Length of output: 3360
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 56-56: Quote this to prevent word splitting.
(SC2046)
[warning] 57-57: Quote this to prevent word splitting.
(SC2046)
[warning] 58-58: Quote this to prevent word splitting.
(SC2046)
[warning] 60-60: Quote this to prevent word splitting.
(SC2046)
[warning] 61-61: Quote this to prevent word splitting.
(SC2046)
[warning] 62-62: Quote this to prevent word splitting.
(SC2046)
This repo is a fork of an open source project (https://github.com/aws/aws-mwaa-local-runner). It looks like they fixed this issues 3 weeks ago: aws#414. I wonder if the better solve is to merge or rebase the latest from the upstream repo. |
What do you think? |
@dkreth, ah…. Good investigation here. As much as I dislike cherry-picking, I think that might be the way to go until we’re ready to upgrade our MWAA instance to 2.9+. |
closed in favor of #14 |
version
11.1
no longer exists on the mirror server we're using, which causes thebootstrap.sh
script to fail (on the 404), which causes the./mwaa-local-env start
command to fail. This can be resolved by using version11.2
instead.I chose 11.2 because it was the next minor version available and worked for me locally, but it may make sense to use the most recent minor version (11.7) instead -- I just haven't tested that yet.
Summary by CodeRabbit
dnf-plugins-core
and building dependencies for Python 3.