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

chore(lib): Updated url with the leader host #100

Merged
merged 9 commits into from
Nov 11, 2024

Conversation

alithethird
Copy link
Contributor

@alithethird alithethird commented Oct 14, 2024

Issue

Since DPE-5200 add leader info to relation data PR we need to update a lot of our(IS DevOps) charms to use the leader-host. We also use the .url property but since it uses the unit ip we have to construct it in the charm.

Solution

Constructing the .url property in the library instead of changing multiple number of charms.

@alithethird alithethird marked this pull request as ready for review October 14, 2024 12:35
@reneradoi
Copy link
Contributor

Hello @alithethird , I tried to look into the issue with the failed integration test. It is failing in the build-step, probably because of the change to the lib. Unfortunately I cannot really debug that without cloning your private repo. Could you please create a branch on this repo here, and push your changes to this new branch? We can also have a session together if that would help you, just let me know.

@alithethird
Copy link
Contributor Author

Hi @reneradoi ! Thank you! I can not create a branch because I do not have write access. My fork should be private but just in case I added you as a collaborator to the fork.

Copy link
Contributor

@reneradoi reneradoi left a comment

Choose a reason for hiding this comment

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

Hey @alithethird, in general we'd be fine with adjusting this, as long as the interface stays the same. But after looking at the failing test, I don't think that this is flaky as you mentioned earlier. I have run the integration tests against the main branch on multiple tries, and there was no failure.

The error from the CI pipeline is here. I suggest you check this and adjust your code accordingly.

@reneradoi
Copy link
Contributor

@alithethird After debugging with a local deployment, I've found the cause of the failing tests and fixed it by updating the pylibjuju to a newer version. Tests are now stable again. Furthermore I've adjusted the logic for the lib changes a little to make them more robust. Please have another look if this is fine for you.

@reneradoi reneradoi self-requested a review November 8, 2024 15:04
@reneradoi reneradoi changed the title chore(lib): Updated url with the chore(lib): Updated url with the leader host Nov 8, 2024
Copy link
Contributor

@Mehdi-Bendriss Mehdi-Bendriss left a comment

Choose a reason for hiding this comment

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

Thanks! Left a minor comment

lib/charms/redis_k8s/v0/redis.py Outdated Show resolved Hide resolved
Co-authored-by: Mehdi Bendriss <[email protected]>
Copy link
Contributor Author

@alithethird alithethird left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks a lot for your time.

@reneradoi reneradoi merged commit 18d36ad into canonical:main Nov 11, 2024
9 checks passed
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