internal: Persist author in site config by indradhanush · Pull Request #46150 · sourcegraph/sourcegraph-public-snapshot

indradhanush · 2023-01-05T11:54:39Z

Part of #46031. Follow up on #45618.

Why

We want to be able to show the site admin who made a specific change to the config.

Test plan

Tested locally.

(Wanted to upload the GIF but its more than 10MB and its 2022 and GitHub restricts file size to 10MB. 😭)

Builds pass.

sashaostrikov

LGTM, but I would remove all the pointers because they are in fact unused and just transformed to zero value of int32 🤔

sashaostrikov · 2023-01-05T13:07:41Z

@@ -0,0 +1,2 @@
+ALTER TABLE critical_and_site_config
+      ADD COLUMN IF NOT EXISTS author_user_id integer;


I'm thinking about adding an index right away.
WDYT?

And I think we keen to adding comments to columns. It would be nice to include what NULL author means.

I'm thinking about adding an index right away.

Is there a use case in your mind about using the index? If we only need to list the changes, then we don't need the index.

And I think we keen to adding comments to columns. It would be nice to include what NULL author means.

We could add some comment about NULL meaning a likely a system start up that updated the config but it would be also misleading since we're just adding this column and I don't want to mislead users for historical columns that don't have the value set to NULL. Not really sure what's the best path here. 🤔

I'm thinking about adding an index right away.
WDYT?

I don't think it's necessary right now, since we likely won't be querying by author for foreseeable future.

But yeah, while we're here, we can add it.

I'd also add a comment, yep.

COMMENT ON COLUMN critical_and_site_config.author_user_id IS 'Horsegraph was here.';

@indradhanush I mean... it won't hurt and we have a bunch of initiatives about improving admin experience. I can easily imagine one day having a filter "by user" for site/code host config changes TBH.

I mean... it won't hurt and we have a bunch of initiatives about improving admin experience. I can easily imagine one day having a filter "by user" for site/code host config changes TBH.

Well, indexes do grow on trees 😝.

I don't want to start building for a hypothetical scenario and end up with premature optimization. If we do end up building this in the future, it should be fairly easy to add an index concurrently for this table, especially since this table is expected to have low-write volume.

indradhanush · 2023-01-05T13:29:05Z

LGTM, but I would remove all the pointers because they are in fact unused and just transformed to zero value of int32 🤔

@sashaostrikov We need to set author_user_id to NULL in some cases which is where we pass a nil instead. Are you suggesting to drop the pointer and pass 0 instead? If that's the case I think nil is better conceptually. 🤔

mrnugget

Because I'm always a bit scared about that: have you tested this manually with a fresh instance that gets booted up with SITE_CONFIG_FILE?

mrnugget · 2023-01-05T13:32:10Z

@@ -0,0 +1,2 @@
+ALTER TABLE critical_and_site_config
+      ADD COLUMN IF NOT EXISTS author_user_id integer;


I'm thinking about adding an index right away.
WDYT?

I don't think it's necessary right now, since we likely won't be querying by author for foreseeable future.

But yeah, while we're here, we can add it.

I'd also add a comment, yep.

COMMENT ON COLUMN critical_and_site_config.author_user_id IS 'Horsegraph was here.';

mrnugget · 2023-01-05T13:37:30Z

@sashaostrikov We need to set author_user_id to NULL in some cases which is where we pass a nil instead. Are you suggesting to drop the pointer and pass 0 instead? If that's the case I think nil is better conceptually. 🤔

I would agree with you in other projects, but here we often use 0 for "no value", because 0 is never ever a valid value, meaning it can be used as "no value" and safes us from nil pointer parties

indradhanush · 2023-01-06T06:33:38Z

Because I'm always a bit scared about that: have you tested this manually with a fresh instance that gets booted up with SITE_CONFIG_FILE?

That's a great call out. I just tested it on a fresh instance with SITE_CONFIG_FILE and it loads up without any issues. And for changes from the UI, the author_user_id is recorded correctly.

Anyhow, it's a Friday and I'll merge it next week.

Note to self: Run main-dry-run before merging.

sashaostrikov

Latest changes LGTM

indradhanush · 2023-01-09T04:45:22Z

Rebased off latest main. main-dry-run now running here: https://github.com/sourcegraph/sourcegraph/tree/main-dry-run/ig/persist-author-site-config

cla-bot Bot added the cla-signed label Jan 5, 2023

indradhanush force-pushed the ig/persist-author-site-config branch from f3b625d to c10e356 Compare January 5, 2023 12:25

indradhanush marked this pull request as ready for review January 5, 2023 12:41

indradhanush requested a review from a team January 5, 2023 12:41

indradhanush added site-admin Site admin experience feature Tracking issues for a feature site-admin-ux Issues related to site-admin UX: bugs, papercuts, design, ... feature-request and removed feature Tracking issues for a feature labels Jan 5, 2023

indradhanush mentioned this pull request Jan 5, 2023

Add reason to site history config #46153

Closed

sashaostrikov reviewed Jan 5, 2023

View reviewed changes

mrnugget approved these changes Jan 5, 2023

View reviewed changes

indradhanush mentioned this pull request Jan 6, 2023

Persist edit history for site configuration including author and expose via GraphQL #46031

Closed

6 tasks

sashaostrikov approved these changes Jan 6, 2023

View reviewed changes

indradhanush force-pushed the ig/persist-author-site-config branch from 1279d04 to e601a62 Compare January 9, 2023 04:44

indradhanush added 9 commits January 9, 2023 11:44

migrations: Add column author_user_id to critical_and_site_config

081dc1f

sg generate

2c1321b

Write author_user_id to critical_and_site_config

8222d67

internal/database: Add test for creation with author_user_id

c3b1157

cmd/frontend: Fix test

3d92d2a

migrations: Add comment to author_user_id

af8da69

Drop pointer and pass by value instead

5191776

sg generate

ceffda4

Fix tests

60caff3

indradhanush force-pushed the ig/persist-author-site-config branch from e601a62 to 60caff3 Compare January 9, 2023 06:15

indradhanush merged commit 3569493 into main Jan 9, 2023

indradhanush deleted the ig/persist-author-site-config branch January 9, 2023 06:44

		@@ -0,0 +1,2 @@
		ALTER TABLE critical_and_site_config
		ADD COLUMN IF NOT EXISTS author_user_id integer;

Conversation

indradhanush commented Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Test plan

Uh oh!

sashaostrikov left a comment

Choose a reason for hiding this comment

Uh oh!

sashaostrikov Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

sashaostrikov Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

indradhanush Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

mrnugget Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

sashaostrikov Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

indradhanush Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

indradhanush commented Jan 5, 2023

Uh oh!

mrnugget left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mrnugget Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

mrnugget commented Jan 5, 2023

Uh oh!

indradhanush commented Jan 6, 2023

Uh oh!

sashaostrikov left a comment

Choose a reason for hiding this comment

Uh oh!

indradhanush commented Jan 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

indradhanush commented Jan 5, 2023 •

edited

Loading