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

MDBF-800 Extend package minor upgrade tests to ensure transition #575

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

elenst
Copy link
Member

@elenst elenst commented Sep 28, 2024

… from old bb

Parts 1 and 2: Switching minor upgrade into "all" mode and extending it with additional checks (ldd, requirements, capabilities)

@elenst
Copy link
Member Author

elenst commented Sep 28, 2024

I'm not sure what I'm supposed to do about this "pre-commit" check. We do not need sudo to affect redirect, and replacing it with tee will only flood the output unnecessarily. For the remaining "not quoted" message, quoting it the way they suggest will make rpm command fail everywhere when the option is not usable and thus the variable remains empty.

@RazvanLiviuVarzaru
Copy link
Collaborator

I'm not sure what I'm supposed to do about this "pre-commit" check. We do not need sudo to affect redirect, and replacing it with tee will only flood the output unnecessarily. For the remaining "not quoted" message, quoting it the way they suggest will make rpm command fail everywhere when the option is not usable and thus the variable remains empty.

Hi, @elenst !

Thank you for your contribution! I'll try to review as much as I can on Monday, as I'll be on vacation for the rest of the week.

Just a couple of quick comments to help you resolve the pre-commit issues:

If you're confident that the inputs do not contain spaces, line feeds, glob characters, and similar elements, you can safely ignore SC2086 by adding the following comment above the affected line of code:

# shellcheck disable=SC2086

For redirection, the current user’s shell will be used to write the output to a file. If there are no permission issues at this point, you can safely ignore the warning by adding:

# shellcheck disable=SC2024

Maybe @fauust can have o a quick look on the scripts too.

@elenst elenst force-pushed the elenst-dev branch 2 times, most recently from 44ac526 to 1e0a1c2 Compare September 29, 2024 17:15
Parts 1 and 2: Switching minor upgrade into "all" mode and extending
it with additional checks (ldd, requirements, capabilities)
@elenst
Copy link
Member Author

elenst commented Sep 29, 2024

you can safely ignore the warning

Thanks, done (kind of, at least I've made it pass).

Copy link
Collaborator

@fauust fauust left a comment

Choose a reason for hiding this comment

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

Thanks @elenst for implementing those extra missing checks, it was planned but I did not found time to implement and was not sure how to proceed (MDBF-401).

Overall this looks good but it's not working ATM, see my comments. BTW if you want to test those scripts manually, you can call the script on a clean VM with something like:
./rpm-upgrade.sh https://buildbot.mariadb.org/#/builders/608/builds/1759 (only jq sudo wget are required and a sudo NOPASSWD user).


bb_log_info "Comparing old and new server capabilities ('%have%' variables)"
if ! diff -u ./capabilities.old.cmp ./capabilities.new.cmp ; then
bb_log_error "ERROR: found changes in server capabilities"
Copy link
Collaborator

@fauust fauust Sep 30, 2024

Choose a reason for hiding this comment

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

- bb_log_error "ERROR: found changes in server capabilities"
+ bb_log_err "found changes in server capabilities"

bb_log_info "Comparing old and new ldd output for installed binaries"
# We are not currently comparing it for Columnstore, too much trouble for nothing
if ! diff -U1000 ./ldd-main.old.cmp ./ldd-main.new.cmp | (grep -E '^[-+]|^ =' || true) ; then
bb_log_error "Found changes in ldd output on installed binaries"
Copy link
Collaborator

Choose a reason for hiding this comment

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

sed 's/bb_log_error/bb_log_err/g'

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

if you want to test those scripts manually, you can call the script on a clean VM

That's a great capability, helps a lot, thanks.
Checked:

  • minor and major upgrade on alma8-amd64 10.5 (minor upgrade fails, but on the "right" reasons, due to differences in dependencies and requirements)
  • minor and major upgrade on ubuntu2004-amd64 10.5 (it passes as far as scripts are concerned, but I noticed that the check for mysql_upgrade is fragile, on my very slow VM the test failed several times as it just didn't wait long enough. We'll need to see if it happens in the buildbot, but since it's an old check, it is out of the scope of this PR)
  • minor upgrade on fedora39-amd64 10.6 (failed due to a requirement difference)

@fauust
Copy link
Collaborator

fauust commented Oct 1, 2024

Good! Regarding https://github.com/koalaman/shellcheck/wiki/SC2024, agree, it's a bit strange and we might consider ignore it everywhere if that helps, shellchek is usually very useful but sometimes also wrong :)

minor and major upgrade on ubuntu2004-amd64 10.5 (it passes as far as scripts are concerned, but I noticed that the check for mysql_upgrade is fragile, on my very slow VM the test failed several times as it just didn't wait long enough. We'll need to see if it happens in the buildbot, but since it's an old check, it is out of the scope of this PR)

Let's see how it goes after the merge, I have triggered a 10.5 and 10.11 rebuild on DEV. When @RazvanLiviuVarzaru is back from vacation (next week), we need to decide how we can make these triggers available to everyone on DEV (ATM, triggered by any push on my fork).

@fauust fauust merged commit f4e68db into MariaDB:dev Oct 1, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants