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

Replace current_master for current_primary #78

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

amandahla
Copy link

@amandahla amandahla commented Feb 20, 2024

Issue

Charms using the Redis library that check for inclusive naming fail because of the word "master".

Solution

Replace current_master for current_primary

Copy link
Contributor

@zmraul zmraul left a comment

Choose a reason for hiding this comment

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

This was done because Redis uses a "master-replica" naming scheme.

I would probably avoid changing it on a couple of places related to the lib, and then having the same concept still called "master" all over the charm. In any case, as Tom pointed out, primary is the more common name.

@amandahla
Copy link
Author

I changed only the part related to the library because I was afraid of causing a big impact/too much to review.
I'll change it for primary!

@amandahla amandahla changed the title Replace current_master for current_main Replace current_master for current_primary Feb 21, 2024
@amandahla
Copy link
Author

@zmraul I changed everything now

@delgod
Copy link
Member

delgod commented Feb 22, 2024

@amandahla, a huge thank you for the contribution! Really

Could you please make sure that the charm actually works after the changes?
also, could you make all the tests work?
We cannot merge PR with broken tests or code

Copy link
Contributor

@zmraul zmraul left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

I'm not a 100% sure that I caught them all, but please do not change commands or outputs from commands sent/received from Redis or Sentinel.

src/sentinel.py Outdated Show resolved Hide resolved
tests/integration/test_charm.py Outdated Show resolved Hide resolved
tests/integration/test_scaling.py Outdated Show resolved Hide resolved
tests/integration/test_scaling.py Outdated Show resolved Hide resolved
tests/integration/test_scaling.py Outdated Show resolved Hide resolved
tests/unit/test_charm.py Outdated Show resolved Hide resolved
tests/unit/test_charm.py Outdated Show resolved Hide resolved
tests/unit/test_charm.py Outdated Show resolved Hide resolved
tests/unit/test_charm.py Outdated Show resolved Hide resolved
tests/unit/test_charm.py Outdated Show resolved Hide resolved
@amandahla
Copy link
Author

@amandahla, a huge thank you for the contribution! Really

Could you please make sure that the charm actually works after the changes? also, could you make all the tests work? We cannot merge PR with broken tests or code

Hi, unfortunately I believe the tests are failing for something not related to this PR:
This is setting to use agent version 2.9.43
https://github.com/canonical/redis-k8s-operator/blob/main/.github/workflows/ci.yaml#L42
Error message:
ERROR this client can only bootstrap 3.3 agents

@amandahla
Copy link
Author

@amandahla, a huge thank you for the contribution! Really

Could you please make sure that the charm actually works after the changes? also, could you make all the tests work? We cannot merge PR with broken tests or code
@delgod
I give it a try anyway but no deal:
https://github.com/canonical/redis-k8s-operator/actions/runs/8005322711/job/21864500948?pr=78

Copy link
Contributor

@zmraul zmraul left a comment

Choose a reason for hiding this comment

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

Thanks!
I'll take a look failures when I have a moment, as they seem unrelated to the charm itself.

@@ -132,7 +132,7 @@ def __init__(self, charm, port):

Copy link
Contributor

Choose a reason for hiding this comment

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

LIBPATCH on the lib will need updating to 6

@zmraul
Copy link
Contributor

zmraul commented Apr 17, 2024

Hi @amandahla. Recent changes should fix the CI/tests. Please rebase on main and give it another try!

For the conflicting file, ci.yaml, the changes on main should be kept.

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.

4 participants