Conditionally change the local DB file name #759
Conversation
This PR tests whether the ops library is present in the system to decide the final file name for the local key/value database, in order to avoid conflicts between that library and charmhelpers.
There was a problem hiding this comment.
So I'm generally against this change due to opportunities for regressions and 'papering' over the bug. Essentially charm-helpers 'owns' the file .unit-state.db as it predates the ops framework, and thus there are lots of charms already using that file. If the ops framework needs an SQLite file, then it should have its own filename and not clobber the charm-helper's one (used in classic and reactive charms). This is a hack, and it shouldn't be added to the library.
However, the Storage class initialise function has a path variable to allow multiple Storage objects to co-exist, so the ops framework could use. Or it could set the UNIT_STATE_DB file. Or the charm application code could do either of these actions, as the charm application code is bringing in charm-helpers. If the ops framework is bring in charm-helpers then it shouldn't clobber the file itself. I hope that makes sense.
| # ops framework libraries and charmhelpers end up using the same | ||
| # DB path, which leads to conflicts. | ||
| # See: https://bugs.launchpad.net/charm-ceph-mon/+bug/2005137 | ||
| db_suffix = '.unit-state.db' |
There was a problem hiding this comment.
It's not a 'suffix'; it's a hidden file. e.g. db_suffix isn't quite right; db_file would be more accurate.
| try: | ||
| import ops | ||
| db_suffix = '.unit-state2.db' | ||
| del ops # Don't hold an unneeded reference. | ||
| except: | ||
| pass |
There was a problem hiding this comment.
This makes charm-helpers dependent on ops, which is entirely the wrong way around.
There was a problem hiding this comment.
I disagree that it creates a dependency. It merely checks that a library can be imported; charm-helpers doesn't require ops to be installed for it to work.
I'm mostly in agreement with your comment. However, I think that, although a hack, something like this is a necessary hack. Whether it's done in the ops framework or here is mostly immaterial to me.
That's true (re: the path variable), but at the same time, the charm-helpers library instantiates FWIW, we've taken the approach of setting the |
This is true as it's a charm-helpers class being used in charm-helpers. It would naturally use the default.
The fix isn't valid in charm-helpers as it's the library at the bottom of the heap and provides mechanisms already to use different files. Also, the SQLite file is already called that in (almost?) every classic and reactive charm. This change would say "actually, the new default for existing charms, is this new file, because ops wants this one". That would create confusion. If the ops framework imports charm-helpers directly, then the fix should be done there. Otherwise, it's the responsibility of the charm code (which is importing both charm-helpers and the ops framework) to sort out what it wants (in this case using the UNIT_STATE_DB). Realistically, the ops framework has created this problem, and if the fix isn't carried in charm code, then it should be changed in the ops framework. |
ajkavanagh
added
the
Incomplete
This PR tests whether the ops library is present in the system to decide the final file name for the local key/value database in order to avoid conflicts between that library and charmhelpers.