Closed Bug 1667211 Opened 5 years ago Closed 3 years ago

rework settings

Categories

(Tecken :: General, task, P2)

Product:
Tecken
Component:
General
Type:
task

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: willkg, Assigned: willkg)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

pr 2253: bug 1667211: rework how configuration works
52 bytes, text/x-github-pull-request
Details | Review
pr 2522: bug 1667211: move tecken test config up a level
52 bytes, text/x-github-pull-request
Details | Review
pr 2523: bug 1667211: switch from django-configuration to everett
52 bytes, text/x-github-pull-request
Details | Review
pr 2525: bug 1667211: fix OIDC_RP_CLIENT_ID configuration parsing
52 bytes, text/x-github-pull-request
Details | Review
pr 2526: bug 1667211: fix config dict value parsing
52 bytes, text/x-github-pull-request
Details | Review

Settings is currently done using django-configurations. It's a class hierarchy that difficult to externalize in the documentation in an autodoc way. It's always hard for me to figure out which configuration goes where.

I want to simplify it and switch it to use Everett which automatically generates documentation.

django-configurations uses the imp module which is deprecated. Peter wrote up an issue about it a couple of years ago.

https://github.com/jazzband/django-configurations/issues/190

The imp module isn't long for this world. Further, django-configurations copies the Django settings and that kicks up a bunch of other deprecation warnings in the tests. Going to bump this up to a P2 to fix soon.

Priority: P3 → P2

I got a chunk of this done, but I'm not going to get to the rest of it any time soon.

Assignee: willkg → nobody

Making this block the GCP migration because it'll be a lot easier to set up a new infrastructure if we have a settings system that has better configuration error messages.

Grabbing it to do soon.

Assignee: nobody → willkg
Blocks: 1687804
Status: NEW → ASSIGNED

I created a PR in the infa repo.

I think the order of operations should go like this:

  1. run ./manage.py diffsettings --all --output unified in stage so we know what configuration looked like before anything happened
  2. finish up and land the PR in the infra repo; that requires some values to be set
  3. finish up and land PR 2523
  4. landing PR 2523 will kick off a stage deploy
  5. fix any issues from the deploy
  6. run ./manage.py diffsettings --all --output unified and fix any discrepancies

Jason landed the infra PR.

willkg merged PR #2523: "bug 1667211: switch from django-configuration to everett" in 0281e18.

This should trigger a stage deploy. I'll keep an eye on that.

willkg merged PR #2525: "bug 1667211: fix OIDC_RP_CLIENT_ID configuration parsing" in 0014149.

Previously, OIDC_RP_CLIENT_ID was a SecretValue in server environments and an IntValue in the local dev environment. I screwed up and picked the wrong parsing method. This fixes that.

willkg merged PR #2526: "bug 1667211: fix config dict value parsing" in 95a8069.

UPLOAD_URL_EXCEPTIONS takes a value that needs to be converted to a Python dict. I used json.loads, but then it turns out we use single quotes and that's not valid JSON. This switches the parsing code to use ast.literal_eval which works with single quotes and is better in a million ways.

Stage deployed fine. I checked the settings and everything looks good.

However, one thing we fixed as a typo setting the statsd namespace, so now all the keys are namespaced and the graphs show nothing. We talked about it and decided to endure short-term pain and keep the fix. I'll update graphs on monday. In a month, it won't matter.

Also, building the docs on readthedocs is broken. I wrote up bug #1760308 for that.

This got deployed to prod in bug #1763165. Everything looks like it's working correctly. Marking as FIXED.

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: