Various charm fixes. #195
Various charm fixes. #195
Conversation
alesstimec
force-pushed
the
charm-fixes-02
branch
from
7ea3804
to
41c7c1f
Compare
alesstimec
assigned alesstimec, ale8k, kian99, babakks and mina1460 and unassigned alesstimec, ale8k and kian99
|
the linter configuration is not set to check new changes only, it will fail because it is trying to lint the entire repo. I did the change required to lint new changes only in livepatch-client. I can do it here if you would like |
charms/candid/src/charm.py
Outdated
| } | ||
| else: | ||
| logging.info("db_uri not stored") | ||
| logging.info("missing postgres relation") |
There was a problem hiding this comment.
We should enter a blocked state here and return
There was a problem hiding this comment.
Though granted lower down if the DB connection string is removed we call this function and if we were to return early here we wouldn't stop the Candid snap so it's a bit tricky.
There was a problem hiding this comment.
removed this check as it is not needed.. code calls self._check_config(event) which will return False if postgres connection string is missing and go into Waiting status
| logging.info("setting config values {}".format(config_values)) | ||
| relation = self.model.get_relation("candid") | ||
| if relation: | ||
| peers = [] |
There was a problem hiding this comment.
The scoping of this variable is weird. It's only declared inside this if but we don't assign it to something outside this scope.
There was a problem hiding this comment.
I.e. it doesn't look like it's actually being used.
There was a problem hiding this comment.
i append to it on L161 and use it on L163?
There was a problem hiding this comment.
Ah don't know how I missed that. Looks good.
| logging.info("database uri {}".format(uri)) | ||
| if not event.master or not event.master.uri: | ||
| logging.debug("removing database connection string") | ||
| del self._state.db_uri |
There was a problem hiding this comment.
Maybe in this case calling the stop hook and returning might be good?
There was a problem hiding this comment.
this hook calls _update_config_and_restart which will now stop the service should any config value be missing
|
rebased to run the actions again |
| import json | ||
|
|
||
|
|
||
| def requires_state_setter(func): |
There was a problem hiding this comment.
Nice replacement for repeated state.ready checks.
There was a problem hiding this comment.
yes. but we have to be careful using it.. it is only useful when you're setting data in application's data bucket, which requires the unit to be leader.. if you're setting data in unit's data bucked, you don't need this check
alesstimec
force-pushed
the
charm-fixes-02
branch
from
c59e72e
to
0c0e1f7
Compare
Description
The what and why - include a summary of the change, describe what it does, and include relevant motivation and context.
Engineering checklist
Check only items that apply
Test instructions
Notes for code reviewers