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.

Sign up for GitHub

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

Jump to bottom

DPE-2758 better messaging when no bucket + ceph testing #332

Merged
merged 18 commits into from
Jan 18, 2024
Merged

DPE-2758 better messaging when no bucket + ceph testing #332

merged 18 commits into from
Jan 18, 2024

Conversation

paulomach
Copy link
Contributor

Issue

  1. When configured bucket don't exist on remote, list-backup action will fail to execute.
  2. We are not testing with ceph/radosgw

Solution

  1. Improve error handling and return no backup when bucket don't exist
  2. Add (micro)ceph integration test

@paulomach paulomach changed the title DPE-2758 better messaging when no bucket DPE-2758 better messaging when no bucket + ceph testing Nov 1, 2023
shayancanonical
shayancanonical previously approved these changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@shayancanonical shayancanonical left a comment

Choose a reason for hiding this comment

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

Wonderful! 🚀
Please replicate to vm charm if you can - also bump libpatches for the backups and s3_helpers libs in the vm charm

carlcsaposs-canonical
carlcsaposs-canonical reviewed Nov 1, 2023
View reviewed changes
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
lib/charms/mysql/v0/s3_helpers.py Show resolved Hide resolved
tests/integration/test_backups.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@paulomach paulomach mentioned this pull request Nov 9, 2023
Merged
shayancanonical
shayancanonical approved these changes Nov 10, 2023
View reviewed changes
Copy link
Contributor

@shayancanonical shayancanonical left a comment

Choose a reason for hiding this comment

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

lgtm

lib/charms/mysql/v0/s3_helpers.py Outdated Show resolved Hide resolved
taurus-forever
taurus-forever approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

The fix and test is important for the next stable release of mysql-server => LGTM.
Carl fired proper security questions, the followup PR will be necessary here (additionally to the new test stabilization changes).

lib/charms/mysql/v0/s3_helpers.py Outdated Show resolved Hide resolved
lib/charms/mysql/v0/s3_helpers.py Show resolved Hide resolved
tests/integration/test_backups.py Outdated Show resolved Hide resolved
@paulomach
Copy link
Contributor Author

paulomach commented Dec 7, 2023 •
edited
Loading

Blocked by: canonical/data-platform-workflows#111

@paulomach
Merge branch 'main' of github.com:canonical/mysql-k8s-operator into fix/
f2bd7ef 
dpe-2758-better-messaging-when-no-bucket

# Conflicts:
#	lib/charms/mysql/v0/mysql.py
#	lib/charms/mysql/v0/s3_helpers.py
@carlcsaposs-canonical carlcsaposs-canonical mentioned this pull request Jan 11, 2024
Merged
@paulomach
Copy link
Contributor Author

Unblocked!

carlcsaposs-canonical
carlcsaposs-canonical reviewed Jan 16, 2024
View reviewed changes
lib/charms/mysql/v0/backups.py
Comment on lines -223 to +227
except Exception:
error_message = "Failed to retrieve backup ids from S3"
logger.exception(error_message)
except Exception as e:
error_message = (
e.message if hasattr(e, "message") else "Failed to retrieve backup ids from S3"
)
logger.error(error_message)
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using a more specific exception type instead

carlcsaposs-canonical
carlcsaposs-canonical requested changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@carlcsaposs-canonical carlcsaposs-canonical left a comment

Choose a reason for hiding this comment

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

please keep data-platform-workflow versions in .github/workflows in sync with pyproject.toml

otherwise, looks good

@paulomach paulomach merged commit 7a3570a into main Jan 18, 2024
28 of 30 checks passed
@paulomach paulomach deleted the fix/dpe-2758-better-messaging-when-no-bucket branch January 18, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlcsaposs-canonical carlcsaposs-canonical carlcsaposs-canonical approved these changes

@shayancanonical shayancanonical shayancanonical approved these changes

@taurus-forever taurus-forever taurus-forever approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@paulomach @taurus-forever @shayancanonical @carlcsaposs-canonical