[DPE-3593] Only check config values against the DB in on_config_changed #395
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #395 +/- ##
==========================================
+ Coverage 80.83% 81.06% +0.22%
==========================================
Files 10 10
Lines 2244 2250 +6
Branches 362 362
==========================================
+ Hits 1814 1824 +10
+ Misses 356 351 -5
- Partials 74 75 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I believe idea of hardcoding option values in charm code is a bad idea, but it is a Marcelo call as PostgreSQL Lead here. I believe it is to create a quick fix for the customer and find a time to invent the proper fix.
BTW, I believe we should check them on config_update event only and defer if PostgreSQL is not running.
| logger.debug("Defer on_config_changed: cluster not initialised yet") | ||
| event.defer() |
There was a problem hiding this comment.
Deferring, because otherwise we'll never validate the config.
There was a problem hiding this comment.
Curiosity question to simplify the code:
can we improve the ops lib to print DEBUG message if defer() got comment?
It will be only:
event.defer("Defer on_config_changed: cluster not initialized yet")
There was a problem hiding this comment.
A reason message could potentially be added in https://github.com/canonical/operator/blob/main/ops/framework.py#L244.
| except psycopg2.OperationalError: | ||
| logger.debug("Defer on_config_changed: Cannot connect to database") | ||
| event.defer() | ||
| return |
There was a problem hiding this comment.
If can't connect, defer
| not in self.postgresql.get_postgresql_text_search_configs() | ||
| ): | ||
| raise Exception( | ||
| raise ValueError( |
There was a problem hiding this comment.
Switching to ValueError to resuse the handler for config blocking
dragomirp
changed the title
| logger.debug("Defer on_config_changed: cluster not initialised yet") | ||
| event.defer() |
There was a problem hiding this comment.
Curiosity question to simplify the code:
can we improve the ops lib to print DEBUG message if defer() got comment?
It will be only:
event.defer("Defer on_config_changed: cluster not initialized yet")
…ed (canonical#395) * Don't check the DB if config values are the defaults * Unit test * Move config DB validation in _on_config_changed
_validate_config_optionsconnecting to the database will be done only in on_config_changed