-
Notifications
You must be signed in to change notification settings - Fork 18
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
DPE 71 Patroni resource install #4
Conversation
db46588
to
cd8f838
Compare
cd8f838
to
6f0ef9e
Compare
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.
Overall looking good, thanks Marcelo!
@@ -73,4 +73,10 @@ deps = | |||
psycopg2-binary | |||
-r{toxinidir}/requirements.txt | |||
commands = | |||
# Download patroni resource to use in the charm deployment. | |||
sh -c 'stat patroni.tar.gz > /dev/null 2>&1 || curl "https://github.com/zalando/patroni/archive/refs/tags/v2.1.3.tar.gz" -L -s > patroni.tar.gz' |
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.
Could this be some sort of fixture, or downloaded in the pytest code using urllib then cleaned up after the test?
The benefit here, is that once you have an uploaded version on chamrhub, you can fetch the resource straight from Charmhub using the API and get a bit more certainty in the test? (Perhaps a future PR?)
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.
Yes, it can be, Jon. Nice suggestion! I'll create a new PR to change this that and upload the resource to Charmhub to have this improved flow of testing with the resource. I created an issue to remind about it.
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.
Great stuff! Amazing PR description and some super thorough commenting. Also thanks for making the PR into two chunks, this is super manageable.
Just have one concern about error handling in install, feel free to PM me if you want to chat about it 😄 Other than that, it looks great!
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.
LGTM, considering Mia's comments
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.
Looks great to me! I like how you handled those cases on failure to install. Great work.
Made some suggestions to the tests but if you disagree then feel free to ignore my suggestions :)
b7b28b7
to
fac4ee2
Compare
* Add Patroni resource * Correctly handle calls from on install hook
Issue
In short this PR partially address JIRA ticket DPE-71. The remaining implementation is split in three separate PRs to make the review easier.
In long:
Solution
Context
Testing
tox.ini
file and the deploy integration test to download and add Patroni as a resource (the successful install of the resource will be better tested in the next PR, where the bootstrap process is switched to use Patroni)Release Notes
apt update
unit test matching the implementation of thetest_install_pip_packages
unit test