search

sourcegraph / sourcegraph-public-snapshot

Code AI platform with Code Search & Cody
https://sourcegraph.com
Other
10.12k stars 1.29k forks source link

Site config: make it easier to remove config variables #32070

Open camdencheek opened 2 years ago

camdencheek commented 2 years ago

Background: Our site config is defined with JSON Schema. We use this schema to validate user-provided settings. We also use this schema to generate go struct representations of our settings using go-jsonschema-generator (built in-house).

Problem: Because site config is validated against the JSON schema definition, we can never safely remove settings since it's possible a customer will have that setting configured, causing errors when validating their settings because they provided a field that does not exist in the schema. This is annoying from a dev perspective because 1) it discourages putting new features behind a setting because those flags cannot be removed, and 2) it causes bloat in our settings structs by filling it over time with deprecated fields.

If someone has a way to fix the first problem, the second problem becomes obsolete. However, I think we can I think we can fix the second problem by adding an "ignore" extension to go-jsonschema-generator.

The generator already uses custom extensions to JSON Schema to support specifying the go type that should be generated. I think it should be possible to add a new extension to the "!go" extensions that basically means "ignore this when generating the go structs."

A possible solution the first problem might include: 1) Mark the field as deprecated 2) Ignore the field when generating go structs 3) Remove any fields from the config that don't exist in the go structs (maybe just a marshalling roundtrip? could be difficult with JSONC and preserving field order). 4) On a subsequent version upgrade, we can remove the field from the JSON schema since it's guaranteed that our upgrades are done in order (no skipping upgrades).

yowayb commented 2 years ago

I think this lends itself to a more approachable adaptive UI, though I'm not a UI engineer so I don't know how hard this is, but was imagining marking those fields deprecated, forking the generator to emit React, and hiding deprecated fields by default unless they have it set. I would also expect to retain old settings indefinitely. I don't think it's at all a problem for the underlying config data to grow quite large, but we shouldn't flood admins with settings.