Added Basic Charm #1
Conversation
Added basic pgbouncer charm based on pgbouncer container found at https://github.com/canonical/pgbouncer-container (currently available in a PR) fixed/removed tests as necessary - coverage will be added back in future commits added some basic action stubs I'm still working on added some simple default config files to ensure pgbouncer behaves as expected on startup install and config-changed hooks updated readme
added basic action stubs added bare minimum config options to get container running documented build process, including installing, building, and adding container to microk8s container registry tested and removed peer replication removed default config files added some documentation to readme implemented config/user management cleaned up integration tests added unit tests, coverage is now 93%
Added and tested an action each to add and remove users, update passwords, and get the list of existing usernames. fixed contributing.md docs updated links in readme.md added hooks and unit tests for each action streamlined config management and added a function stub to restart pgbouncer, once we start work on ensuring the health of the pgbouncer process added checks to not update config unnecessarily added better user management - a user can now explicitly set the user list through the config. increased randomly generated password length removed unnecessary integration test stubs
Users stored in a local dict were being overwritten, so now they're parsed in from the existing userlist.txt, modified where necessary, and then rewritten. This should be updated to be more efficient. Updated tests accordingly
restructured charm.py updated some method names to make more sense to a reviewer Updated docstrings Some minor updates to tests, including testing that changes in config propagate to local config files
WRFitch
requested review from
MiaAltieri,
jnsgruk,
marceloneppel,
quiet-ranger,
zmraul,
DnPlas,
zeljkoagic and
marcoppenheimer
…r action mandatory Also renamed change-password action updated a template variable in contributing.md
updated ops version in requirements.txt Handled empty userlist.txt files Documented user management integration test updated on_install unit test to verify that it runs on.install, as configured in the charm's constructor. tested handling of empty userlist.txt files.
added contributor agreement docs to contributing.md added license and security sections to readme ensured CI only runs on push to branch
Passwords are now hashed using md5 by default tests updated to match updated passwords cleaned up integration tests
|
LGTM! Thanks Will! |
|
Hey! lots of nice work here, its great to see all of this, sorry I took so long in the review. Overall I am quite impressed and things are looking good 😎 , but I just have some concerns. I was a little confused about the TODOs, are the functions with TODOs finished? If not maybe they should be removed/saved for a future PR. (or alternatively you could add "in future patch" after them). Also some of the TODOs are long, it might be useful to link them to a JIRA or issue. But I am not sure what the best practice is here. I think it would be good to get the opinion of someone more senior than me since I am new here :) (because after all this might be a fine thing to do, but I don't really know what the call here is 🤷 ) (Side note these todos show that you're really thinking through things, some of them might make useful spec documents)
Also some small things that need to be done:
|
formatting removing unnecessary TODO comments added environment setup docs documented actions in readme type hinting better docstrings Still haven't fixed the integration test yet - for one reason or another, accessing files from machines created during an integration test is proving difficult.
|
|
||
| https://discourse.charmhub.io/t/4208 | ||
| """ | ||
| """Charmed PgBouncer connection pooler.""" |
There was a problem hiding this comment.
Some parts of the code are a little confusing to me (since this a lot of this is new to me). I think some of it could use some comments before a code block when it isn't obvious whats going on. (IMHO)
But if you disagree then that is also fine. I can leave comments where I got confused if you want to add comments (feel free to add all/some/none).
There was a problem hiding this comment.
Yes Mia! Clarifying comments in code is one of the hallmarks of good coding. Also, please do not fall into the trap of translating to English what is written in code.
src/charm.py
Outdated
| } | ||
| ) | ||
| return | ||
| password = hashlib.md5(event.params["password"].encode()) |
There was a problem hiding this comment.
I've extracted hashing into a separate function, which is independently documented. Does this adequately explain the function of the hashing, or shall I elaborate?
src/charm.py
Outdated
| if current not in self._stored.things: | ||
| logger.debug("found a new thing: %r", current) | ||
| self._stored.things.append(current) | ||
| users = self._get_userlist_from_container() |
Apologies for the size of this PR; future ones should be smaller and more incremental, and shouldn't have a bunch of user management stuff grafted on.