oobmigration: derive subscription account number and extract license key fields by unknwon · Pull Request #39344 · sourcegraph/sourcegraph-public-snapshot

unknwon · 2022-07-25T06:36:19Z

This PR adds two OOB migrations:

Attempt to extract the account number from the associated user of subscriptions, sister PR: https://github.com/sourcegraph/sourcegraph/pull/39209
Extract license fields from the license key text and store them to dedicated columns, sister PR: https://github.com/sourcegraph/sourcegraph/pull/39215

Test plan

Manually e2e tested locally.

Part of https://github.com/sourcegraph/sourcegraph/issues/38661

…key fields

unknwon · 2022-07-25T06:38:39Z

-		uuid, userID, dbutil.NewNullString(accountNumber),
+		uuid, userID, accountNumber,


If we continue inserting NULL values for subscriptions that don't have an account number, it confuses the OOB migration progress checking. Admittedly this is a shortcut, but I think it is fine, since having the account_number column NULL-able was for the sake of backward compatibility.

We could add a column “is_migrated boolean” or something like that to circumvent it. I think it would be good practice to actually write nulls instead of empty strings, wdyt?

Technically we could, and agree with you in general!

But frankly speaking for this particular task, I think even an OOB migration is somewhat an overkill because this only matters for the dotcom instance and I could have just written a script that writes to update the DB (but that's super risky so... OOB migration seems safer with code review and less error-prone).

I'm not incentivized enough to make this part of the migration long-lasting. 😅 WDYT?

Is okay, I’m just thinking about when we can deprecate the migrator and how we would eventually get to 1 state for “not set” again, preferably NULL.

Not saying that's the optimal approach, but by that time, a sync migration that does update product_subscription set account_number = null where account_number = '' would solve it 🤫 OR maybe we have migrated license management out of the dotcom (completely rewritten).

Sounds good :) I don't believe in the latter yet, but the former satisfies me enough 😆

sourcegraph-bot · 2022-07-25T06:40:53Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff fd4f511...6b7654b.

Notify	File(s)
@efritz	enterprise/cmd/worker/main.go internal/oobmigration/oobmigrations.yaml

eseliger

Approving but one open question whether one of them has to be OOB and would be nice to see a test for this :)

eseliger · 2022-07-26T07:37:50Z

-		uuid, userID, dbutil.NewNullString(accountNumber),
+		uuid, userID, accountNumber,


We could add a column “is_migrated boolean” or something like that to circumvent it. I think it would be good practice to actually write nulls instead of empty strings, wdyt?

eseliger · 2022-07-26T07:44:32Z

+
+		var accountNumber string
+		if i := strings.LastIndex(username, "-"); i > -1 {
+			accountNumber = username[i+1:]


Since this is all the logic it takes, it feels like this migration could get away with a simple synchronous DB migration, as on prem won’t ever have entries in this table and dotcom only has a couple thousand, it doesn’t feel like unbearable load. Negative effect of an OOB
migrator is that you can never assume the data is in a certain shape until it’s been deprecated.

Cc @efritz for some more thoughts.

Negative effect of an OOB
migrator is that you can never assume the data is in a certain shape until it’s been deprecated.

Yes, but I think this negative effect is fine in this case, the Sales data pipeline does not have this assumption (given the history of multiple broken states for license data) thus supporting continuously filling data gaps (even if this happens). 🤣

eseliger · 2022-07-26T07:48:28Z

@@ -0,0 +1,221 @@
+package productsubscription


A test for each of these would be appreciated to prove they report progress correctly (especially on non-dotcom) and to check if it properly decodes the license.

Is there a good example of how to write the test for OOB migrations? 🙏

Oh NVM, I think these tests should be easy to write!

I think some of the migrators have a good test suite attached! (I know some in batches should, at least)

efritz

👍

@unknwon A few questions I didn't get quickly from resources in this PR:

I'm not familiar with the tables in question here. What writes to them (in particular, are they writing only the new format according to this migration, or are they still writing an old format that must be caught by this process)?
Does this apply only to the dotcom instance? Why (Maybe answering what fills this table also answers this question)?

unknwon · 2022-07-27T06:13:40Z

@efritz:

I'm not familiar with the tables in question here. What writes to them (in particular, are they writing only the new format according to this migration, or are they still writing an old format that must be caught by this process)?

All writes to them are only using the new format (changes made in the linked sister PRs), these OOB migrations only intend to deal with existing data.

Does this apply only to the dotcom instance? Why (Maybe answering what fills this table also answers this question)?

Yes, only the dotcom Postgres stores product subscriptions and licenses.

unknwon · 2022-07-27T08:14:18Z

Thanks everyone! Merging as-is, happy to address any post-merge comments!

unknwon added 2 commits July 25, 2022 14:31

oobmigration: derive subscription account number and extract license …

17b7651

…key fields

Fix tests

d4124d3

cla-bot Bot added the cla-signed label Jul 25, 2022

github-actions Bot added the team/iam label Jul 25, 2022

unknwon commented Jul 25, 2022

View reviewed changes

unknwon marked this pull request as ready for review July 25, 2022 06:39

unknwon requested review from a team and efritz July 25, 2022 06:39

eseliger approved these changes Jul 26, 2022

View reviewed changes

introduced_version_minor: 43

686a427

efritz approved these changes Jul 26, 2022

View reviewed changes

Comment thread enterprise/internal/productsubscription/migrations.go Outdated

unknwon added 3 commits July 27, 2022 14:31

Simplify Up logic to a single query based on feedback

45b9387

Add tests

2352376

Fix lint error

6b7654b

unknwon merged commit b4bb2d2 into main Jul 27, 2022

unknwon deleted the jc/subscriptions-licenses-oob-migration branch July 27, 2022 08:14

		uuid, userID, dbutil.NewNullString(accountNumber),
		uuid, userID, accountNumber,

Conversation

unknwon commented Jul 25, 2022

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sourcegraph-bot commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eseliger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unknwon Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efritz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

unknwon commented Jul 27, 2022

Uh oh!

unknwon commented Jul 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sourcegraph-bot commented Jul 25, 2022 •

edited

Loading

unknwon Jul 26, 2022 •

edited

Loading