Fixes 4741: associate snapshots to templates directly #836

xbhouse · 2024-10-01T14:51:54Z

Summary

Associates snapshots to templates by adding snapshot_uuid to the templates_repository_configurations table
Migrates any existing templates' snapshots to this table
Simplifies the query for listing a template's snapshots to use the template-snapshot association
Checks that repos added to a template have a snapshot and rejects request if they do not

Testing steps

To test the migration:

In main, create 1 custom repo and 1 RH repo
Change the URL of the custom repo so you have 2 snapshots, and update the snapshot's created_at dates to be on different days
Change the URL one more time to get another snapshot and don't update this date
Create 3 templates: 1 with a date set to be between the dates of the snapshots, 1 with a date before the snapshots, and 1 using latest content
Check out this PR and run the migration. Verify the correct snapshot uuids were added to the templates_repository_configurations table (you can either check the table directly or check via the GET /templates/:uuid/snapshots/ endpoint)

To test a template's snapshots are updated correctly:

Create a custom repo and a RH repo, let them snapshot, and create a template with these repos with use_latest set to true
Listing the template's snapshots (GET /templates/:uuid/snapshots/) should still work as expected
Add / remove one of the template's repos, listing the template's snapshots show updated snapshots
Trigger another snapshot of one of the repos in template. Once this task completes you should see that latest snapshot when listing the template's snapshots

jlsherrill · 2024-10-07T17:36:14Z

https://issues.redhat.com/browse/HMS-4741

jlsherrill · 2024-10-07T20:28:56Z

db/migrations/20240930100434_add_snap_uuid_to_templates.up.sql

+DROP CONSTRAINT IF EXISTS fk_templates_repository_configurations_snapshots,
+ADD CONSTRAINT fk_templates_repository_configurations_snapshots
+FOREIGN KEY (snapshot_uuid) REFERENCES snapshots(uuid)
+ON DELETE SET NULL;


One question is whether it should restrict the deletion or set the column null. If we restricted the deletion, it would require all snapshots to be updated for the template prior to deleting the snapshot.

If we allow for null, we could potentially end up with a null value. What would be the ramifications of that?

i think restricting the deletion instead makes sense, especially if we don't allow the snapshot_uuid to be null (see reply further down). and before deleting snapshots we should be updating the snapshots in the template anyway (looking at the ticket for delete and bulk-delete)

jlsherrill · 2024-10-07T20:30:26Z

pkg/dao/snapshots.go

 		Select("snapshots.*, STRING_AGG(repository_configurations.name, '') as repo_name").
-		Where("repository_configuration_uuid IN ?", template.RepositoryUUIDS).
+		Joins("JOIN templates_repository_configurations ON templates_repository_configurations.snapshot_uuid = snapshots.uuid").
+		Where("snapshots.repository_configuration_uuid IN ?", template.RepositoryUUIDS).
 		Where("repository_configurations.name ILIKE ?", fmt.Sprintf("%%%s%%", repositorySearch)).


It looks like these lines are repeated between the count query and the actual query:

readableSnapshots(sDao.db.WithContext(ctx), orgID). Joins("JOIN templates_repository_configurations ON templates_repository_configurations.snapshot_uuid = snapshots.uuid"). Where("snapshots.repository_configuration_uuid IN ?", template.RepositoryUUIDS). Where("repository_configurations.name ILIKE ?", fmt.Sprintf("%%%s%%", repositorySearch))

you can build a base query struct and re-use it for both

jlsherrill · 2024-10-07T20:39:36Z

db/migrations/20240930100434_add_snap_uuid_to_templates.up.sql

@@ -0,0 +1,11 @@
+BEGIN;
+
+ALTER TABLE templates_repository_configurations ADD COLUMN IF NOT EXISTS snapshot_uuid UUID DEFAULT NULL;


This question kinda goes to the question below about the ramifications around a null value here.

Would it be possible to (eventually) have the column be not null? (it might take more than 1 PR to pull this off).

good point! i made the default snapshot_uuid value null because in the API we allow template creation with repositories without snapshots. if we wanted to make this not null, we'd have to validate that all repositories have snapshots before they're added to a template i think

I think we'd also need to migrate any existing snapshots that are in templates to be included on the table

jlsherrill · 2024-10-14T15:27:18Z

db/migrations/20240930100434_add_snap_uuid_to_templates.up.sql

+FOREIGN KEY (snapshot_uuid) REFERENCES snapshots(uuid)
+ON DELETE RESTRICT;
+
+COMMIT;


do we want to make the snapshot_uuid column not null?

i think so, do you think we should do that here or in a following PR?

to do that we'd need to make sure all repos have snapshots before they're added to a template (which i've added in here). we'd also need to set not null on the column and the model, and since there would be a few seconds where the column would be null until the update-template-content task runs and adds in the snapshot uuids, creating a template would fail. so we'd probably need to add a method to insert the snapshots before the template is created. (and there might be some additional steps that i'm missing)

pushed up these changes!

xbhouse · 2024-10-16T16:03:24Z

pkg/dao/snapshots_test.go

@@ -620,33 +620,6 @@ func (s *SnapshotsSuite) TestListByTemplate() {
 	assert.Equal(t, repoConfig2.UUID, snapshots.Data[1].RepositoryUUID)
 }

-func (s *SnapshotsSuite) TestListByTemplateNoRepos() {


removed this test because templates should now always have snapshots

jlsherrill · 2024-10-16T20:01:20Z

db/migrations/20240930100434_add_snap_uuid_to_templates.up.sql

@@ -1,6 +1,6 @@
 BEGIN;

-ALTER TABLE templates_repository_configurations ADD COLUMN IF NOT EXISTS snapshot_uuid UUID;
+ALTER TABLE templates_repository_configurations ADD COLUMN IF NOT EXISTS snapshot_uuid UUID NOT NULL;


you'd have to set it to NOT NULL after the update below

in fact you may want to do the update to set the values, and then delete any null row to be safe and only then set it to NOT NULL.

ohhh right, makes sense! just pushed up that fix

jlsherrill · 2024-10-18T17:48:39Z

overall this seemed to work fine. I did see one error though if i:

create a repo
create a template and add the repo
delete the repo

i get an error:

1:46PM WRN [Finished Task] task failed error="ERROR: update or delete on table \"snapshots\" violates foreign key constraint \"fk_templates_repository_configurations_snapshots\" on table \"templates_repository_configurations\" (SQLSTATE 23503)" request_id=4c0220f1-94d8-44cf-819d-6ef213c4c11c task_id=5c876540-ea47-4703-813e-bcd312e7ff87 task_type=delete-repository-snapshots

xbhouse · 2024-10-18T19:10:09Z

overall this seemed to work fine. I did see one error though if i:

create a repo
create a template and add the repo
delete the repo

i get an error:

1:46PM WRN [Finished Task] task failed error="ERROR: update or delete on table \"snapshots\" violates foreign key constraint \"fk_templates_repository_configurations_snapshots\" on table \"templates_repository_configurations\" (SQLSTATE 23503)" request_id=4c0220f1-94d8-44cf-819d-6ef213c4c11c task_id=5c876540-ea47-4703-813e-bcd312e7ff87 task_type=delete-repository-snapshots

ah good catch. because we're restricting deletion we'll need to delete any rows from template_repo_configs with that repo's snapshot uuids before the repo is deleted. then the snapshot integration test updates are probably not necessary. fixing!

jlsherrill

ACK!

xbhouse force-pushed the 4741 branch 4 times, most recently from aca123c to 6255c8d Compare October 1, 2024 21:21

xbhouse marked this pull request as ready for review October 2, 2024 15:38

xbhouse force-pushed the 4741 branch 2 times, most recently from a5a308b to 76d1694 Compare October 2, 2024 17:11

jlsherrill added the qe-testing-needed label Oct 7, 2024

jlsherrill reviewed Oct 7, 2024

View reviewed changes

xbhouse force-pushed the 4741 branch 3 times, most recently from 765ce2f to bdb8359 Compare October 10, 2024 22:11

dominikvagner mentioned this pull request Oct 11, 2024

Fixes 4742: snapshot and bulk delete endpoints and task #845

Merged

xbhouse force-pushed the 4741 branch from 4b96051 to fcf00b9 Compare October 14, 2024 14:04

jlsherrill reviewed Oct 14, 2024

View reviewed changes

jlsherrill added no-qe-needed and removed qe-testing-needed labels Oct 14, 2024

xbhouse commented Oct 16, 2024

View reviewed changes

jlsherrill reviewed Oct 16, 2024

View reviewed changes

xbhouse added 5 commits October 18, 2024 15:45

Fixes 4741: associate snapshots to templates directly

f4e62a5

make snapshot_uuid not null, insert uuids on create, fix tests

f6b4c41

lint

1b418bf

fix migration

cadce25

remove snaps from trc table before deleting

60d901b

xbhouse force-pushed the 4741 branch from 8798f4d to 60d901b Compare October 18, 2024 20:04

jlsherrill approved these changes Oct 18, 2024

View reviewed changes

swadeley merged commit 629a778 into content-services:main Oct 21, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes 4741: associate snapshots to templates directly #836

Fixes 4741: associate snapshots to templates directly #836

xbhouse commented Oct 1, 2024 •

edited

Loading

jlsherrill commented Oct 7, 2024

jlsherrill Oct 7, 2024 •

edited

Loading

xbhouse Oct 8, 2024

jlsherrill Oct 7, 2024

jlsherrill Oct 7, 2024

xbhouse Oct 8, 2024

rverdile Oct 8, 2024

jlsherrill Oct 14, 2024

xbhouse Oct 14, 2024

xbhouse Oct 16, 2024

xbhouse Oct 16, 2024

jlsherrill Oct 16, 2024

xbhouse Oct 17, 2024

jlsherrill commented Oct 18, 2024

xbhouse commented Oct 18, 2024

jlsherrill left a comment

		@@ -0,0 +1,11 @@
		BEGIN;

		ALTER TABLE templates_repository_configurations ADD COLUMN IF NOT EXISTS snapshot_uuid UUID DEFAULT NULL;

Fixes 4741: associate snapshots to templates directly #836

Fixes 4741: associate snapshots to templates directly #836

Conversation

xbhouse commented Oct 1, 2024 • edited Loading

Summary

Testing steps

jlsherrill commented Oct 7, 2024

jlsherrill Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlsherrill commented Oct 18, 2024

xbhouse commented Oct 18, 2024

jlsherrill left a comment

Choose a reason for hiding this comment

xbhouse commented Oct 1, 2024 •

edited

Loading

jlsherrill Oct 7, 2024 •

edited

Loading