Re-add database user and password as settings, and rename from MYSQL_* to SQL_*

[nickeopti]

The SQLAlchemy broke setting MYSQL_USER and MYSQL_PASSWORD settings. This PR fixes that.

It also renames the fields from MYSQL_* to the more generic SQL_*, in anticipation of the coming PostgreSQL driver (and potential future others). This is currently just a plain rename, thus breaking compatibility with deployments using the previous fields. We need to decide whether that is okay, or whether there should be some deprecation warning system first.

Ideally, we also ought to make some tests for this. I have apparently previously broken this, without noticing. That shouldn't be possible in the future. Feel free to just add such tests -- it may take a while before I'm back to do it.

[codecov]

Codecov Report

Merging #258 (a82d805) into main (343a946) will increase coverage by 0.20%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #258      +/-   ##
==========================================
+ Coverage   98.57%   98.77%   +0.20%     
==========================================
  Files          49       49              
  Lines        2175     2211      +36     
  Branches      304      310       +6     
==========================================
+ Hits         2144     2184      +40     
+ Misses         19       17       -2     
+ Partials       12       10       -2     

Impacted Files	Coverage Δ
terracotta/drivers/relational_meta_store.py	98.93% <ø> (+0.02%)	⬆️
terracotta/config.py	100.00% <100.00%> (ø)
terracotta/exceptions.py	100.00% <100.00%> (ø)
terracotta/drivers/base_classes.py	100.00% <0.00%> (ø)
terracotta/drivers/terracotta_driver.py	100.00% <0.00%> (ø)
terracotta/drivers/sqlite_remote_meta_store.py	100.00% <0.00%> (ø)
terracotta/server/flask_api.py	96.20% <0.00%> (+5.66%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 343a946...a82d805. Read the comment docs.

[mrpgraae]

IMO we should just deprecate the MYSQL_ vars and make the breaking change. We'll want to do that before 1.0 in any case.

If we want to be nice, we can detect MYSQL_ and fail with an error message saying that this has changed to SQL_. Then remove the error message when we release 1.0.

[dionhaefner]

I would prefer a proper deprecation cycle with a warning and then remove it in the next major version. Emitting this warning should be pretty painless. I suspect that we have a good amount of users by now (judging from the traffic and downstream projects), and I don't see any reason to break things unless we need to.

[mrpgraae]

I would prefer a proper deprecation cycle with a warning and then remove it in the next major version. Emitting this warning should be pretty painless. I suspect that we have a good amount of users by now (judging from the traffic and downstream projects), and I don't see any reason to break things unless we need to.

In that case, @nickeopti ignore my approved review and let's add this warning.

I was thinking that since we don't have DB migration yet, people would have to re-deploy when they upgrade in any case, so it's unlikely that this change would unexpectedly break an existing deployment.

[dionhaefner]

It was my understanding that DB migration is a hard requirement before the next release.

[mrpgraae]

It was my understanding that DB migration is a hard requirement before the next release.

Okay, I guess I thought that we would support DB migration from the next release and onwards and not from the current release to the next.

[nickeopti]

I've implemented a deprecation system for the settings. Perhaps a bit over-engineered, but I think it does the job.

One question, though, is whether we shall make the warnings more visible. Apparently the default settings in Python hide DeprecrationWarnings. Is that acceptable, or shall we either use something else or change the filtering levels with something like

warnings.simplefilter('always', DeprecationWarning)

There's also a question of PendingDeprecationWarning versus DeprecationWarning. Essentially, I don't know the best practices regarding deprecation cycles.

I've not added tests for this yet. I'd like to know that the functionality is correct before adding those.

[dionhaefner]

I would have approached this a bit differently. Right now this seems weird to me:

>>> settings = get_settings()
>>> settings.MYSQL_USER = "foo"
>>> settings.MYSQL_USER
AttributeError: 'TerracottaSettings' object has no attribute 'MYSQL_USER'

I would suggest something simpler:

• Leave the deprecated settings as attributes of TerracottaSettings
• Use your deprecation map in pre_load and post_load to emit the warning and ensure that the new setting is written

One question, though, is whether we shall make the warnings more visible. Apparently the default settings in Python hide DeprecrationWarnings.

IMO Python's deprecation warnings are only appropriate for libraries, where you don't want end users to be bothered by them (i.e., where they are only relevant to developers). This is not the case in end-user applications and we should use a different warning that is visible by default. I suggest introducing a new warning class in exceptions.py and using that.

[nickeopti]

I would have approached this a bit differently. Right now this seems weird to me:

>>> settings = get_settings()
>>> settings.MYSQL_USER = "foo"
>>> settings.MYSQL_USER
AttributeError: 'TerracottaSettings' object has no attribute 'MYSQL_USER'

But is that really a concern, as NamedTuples are immutable? So settings.MYSQL_USER = "foo" has never been possible. However, I do see that this code does not currently work with the update_settings, which is certainly a bug on my part.

I would suggest something simpler:

• Leave the deprecated settings as attributes of TerracottaSettings
• Use your deprecation map in pre_load and post_load to emit the warning and ensure that the new setting is written

That makes sense

One question, though, is whether we shall make the warnings more visible. Apparently the default settings in Python hide DeprecrationWarnings.

IMO Python's deprecation warnings are only appropriate for libraries, where you don't want end users to be bothered by them (i.e., where they are only relevant to developers). This is not the case in end-user applications and we should use a different warning that is visible by default. I suggest introducing a new warning class in exceptions.py and using that.

I'll do that.

Another question:
I'm tempted to use a pydantic BaseModel for the settings, as that would allow us to specify the fields as well as validation more compactly, with less repetition of field names, in a single class. I just made a quick mockup, and it seems to roughly halve the number of lines of code, while arguably be easier to understand. Downside being adding a package as well as pydantic models being larger than namedtuples. What is your opinion on that change?

[nickeopti]

Also, when setting a deprecated setting, eg MYSQL_USER, do we expect:

1. that just SQL_USER gets set with that value, or
2. that both MYSQL_USER and SQL_USER gets set with that value?

[dionhaefner]

But is that really a concern, as NamedTuples are immutable? So settings.MYSQL_USER = "foo" has never been possible.

Right, my bad. It should have been update_settings and then reading it back.

I'm tempted to use a pydantic BaseModel for the settings, as that would allow us to specify the fields as well as validation more compactly, with less repetition of field names, in a single class. I just made a quick mockup, and it seems to roughly halve the number of lines of code, while arguably be easier to understand. Downside being adding a package as well as pydantic models being larger than namedtuples. What is your opinion on that change?

I'm not a big fan of introducing a dependency for functionality that is already present and works fine. IMO we have nothing to gain from switching at this point, but if others are positive to it I won't veto it :)

Also, when setting a deprecated setting, eg MYSQL_USER, do we expect:

1. that just SQL_USER gets set with that value, or
2. that both MYSQL_USER and SQL_USER gets set with that value?

I'd go with number 2.

[nickeopti]

I believe it is ready for review @dionhaefner

[dionhaefner]

Thanks @nickeopti !