Fixes 2702,2887: Add orgID to snapshot list endpoint #435

Andrewgdewar · 2023-10-19T16:32:30Z

Summary

HMS-2702

Adds orgID as a requirement to snapshot list

HMS 2887

Allows the package list and snapshotList endpoint to return red hat repository (orgID of "-1") related data.

Additional changes:

Adds the new variable "any" to the "fetchRepoConfig" method.
Updated dao methods using the above function as needed, create/delete/update are now prevented from taking effect on red_hat repositories.

Testing steps

Easiest testing method would be to checkout the accompanied front-end PR here, and follow steps there-in.

jlsherrill · 2023-10-19T17:00:15Z

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

pkg/dao/snapshots.go

jlsherrill · 2023-10-25T22:00:16Z

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

pkg/dao/repository_configs.go

pkg/dao/snapshots_test.go

jlsherrill · 2023-10-31T17:21:27Z

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

rverdile · 2023-11-03T18:47:51Z

I'm not sure why, but it looks like most of the test didn't run. Do you see that too?

swadeley · 2023-11-06T08:15:00Z

I'm not sure why, but it looks like most of the test didn't run. Do you see that too?

Hi, test_validate_repo_parameters failed because the same repo was used in the previous filter test and not cleaned up. I have added another repo with invalid metadata to the filter test to avoid this conflict.

Not sure why test run was incomplete (I see 28 selected and I counted 26 PASSED. Then later I see = 1 failed, 26 passed, 1 skipped, 9 deselected).

swadeley · 2023-11-06T08:16:02Z

/retest

jlsherrill · 2023-11-06T20:14:38Z

/retest

rverdile

It says in your summary that create/update/delete do not have an effect for red hat repositories. Right now, if my org ID is -1, I can delete/update but not create. Obviously, that's not a real use case, but I feel the behavior should probably be consistent unless there's a reason for it not to be.

Do we expect to want to delete/update red hat repositories from the API? Or should that be blocked the way create is blocked?

rverdile · 2023-11-07T18:36:40Z

pkg/dao/repository_configs.go

@@ -587,7 +606,7 @@ func (r repositoryConfigDaoImpl) bulkDelete(tx *gorm.DB, orgID string, uuids []s
 		var err error
 		var repoConfig models.RepositoryConfiguration

-		if repoConfig, err = r.fetchRepoConfig(orgID, uuids[i]); err != nil {
+		if repoConfig, err = r.fetchRepoConfig(orgID, uuids[i], false); err != nil {


If I try to bulk delete a red hat repo, I correctly get the "not found" error. BUT it's returning a 500 response code instead of a 404

I was able to fix this issue, I think we largely just weren't hitting it before, and it was pre-existing.

jlsherrill · 2023-11-07T19:50:48Z

@swadeley we've added a command to run when deploying to ephemeral that should snapshot a small rhel repo (Red Hat Ansible Engine 2 for RHEL 8 x86_64 (RPMs)) Hopefully there's enough memory for it to snapshot. Note that it doesn't 'block' deployment on the snapshot, it happens in the background.

I'll spin it up to verify as well once the image is built

jlsherrill · 2023-11-07T20:33:35Z

I've updated the init container to first wait until pulp is up before triggering the task. I haven't been able to test this in ephemeral, as i can't seem to get the deployment.yaml changes to take effect when pointing to it with my local bonfire config. I'm not 100% sure why.

jlsherrill · 2023-11-07T21:50:39Z

So i got it working, about half the time :/ the other half the time, there is some timing issue and the snapshot task is not scheduled. Pushing up some debugging

swadeley · 2023-11-08T05:50:41Z

@swadeley we've added a command to run when deploying to ephemeral that should snapshot a small rhel repo (Red Hat Ansible Engine 2 for RHEL 8 x86_64 (RPMs)) Hopefully there's enough memory for it to snapshot. Note that it doesn't 'block' deployment on the snapshot, it happens in the background.

I'll spin it up to verify as well once the image is built

Hi, I have had this PR and the frontend [1] deployed for over 24 hours. I saw the Ansible repo was snapshotted right away, but still no RHEL repos due to "Killed by signal 9." as mentioned before [2].

Let me know when I should redeploy to test a new image.

Thank you

[1] content-services/content-sources-frontend#156
[2] content-services/content-sources-frontend#156 (comment)

This reverts commit 9318ada.

jlsherrill · 2023-11-08T15:59:05Z

@swadeley yes, we'll likely not ever see these large rhel repos be snapshotable in ephemeral.

jlsherrill · 2023-11-08T15:59:44Z

@swadeley so this should be good to merge if you are okay with it. I have removed my debugging output

swadeley · 2023-11-09T02:08:05Z

/retest

swadeley · 2023-11-10T01:44:11Z

/retest

Andrewgdewar · 2023-11-10T20:55:37Z

It says in your summary that create/update/delete do not have an effect for red hat repositories. Right now, if my org ID is -1, I can delete/update but not create. Obviously, that's not a real use case, but I feel the behavior should probably be consistent unless there's a reason for it not to be.

Do we expect to want to delete/update red hat repositories from the API? Or should that be blocked the way create is blocked?

@rverdile
If you can highlight somewhere that the dao layer would allow this, please do.
I think I covered all cases of CrUD not being possible.

oh...
I think I see what you mean in your comment now.
if the ordID is -1, then you could delete/update repositories.
So to prevent that we would need to specifically check that is not the orgID
Is that necessary? auth gives us the ordID (not the user) so I couldn't really see this happening.

rverdile

nice!!

jlsherrill added the qe-testing-needed label Oct 19, 2023

Andrewgdewar force-pushed the HMS-2702 branch 2 times, most recently from 1da5d1b to 07951b1 Compare October 19, 2023 20:42

Andrewgdewar marked this pull request as ready for review October 20, 2023 16:40

Andrewgdewar requested review from jlsherrill and rverdile October 23, 2023 15:14

rverdile reviewed Oct 23, 2023

View reviewed changes

pkg/dao/snapshots.go Outdated Show resolved Hide resolved

Andrewgdewar force-pushed the HMS-2702 branch from dd0dd51 to 58bb8a0 Compare October 25, 2023 21:10

Andrewgdewar changed the title ~~Fixes 2702: Add orgID to snapshot list endpoint~~ Fixes 2702,Fixes 2887: Add orgID to snapshot list endpoint Oct 25, 2023

Andrewgdewar mentioned this pull request Oct 25, 2023

Fixes 2533: Add red hat repositories UI content-services/content-sources-frontend#156

Merged

Andrewgdewar force-pushed the HMS-2702 branch from 91cd454 to 12c7cb3 Compare October 27, 2023 18:29

Andrewgdewar requested a review from rverdile October 27, 2023 19:00

rverdile reviewed Oct 30, 2023

View reviewed changes

pkg/dao/repository_configs.go Outdated Show resolved Hide resolved

rverdile reviewed Oct 30, 2023

View reviewed changes

pkg/dao/repository_configs.go Outdated Show resolved Hide resolved

rverdile reviewed Oct 30, 2023

View reviewed changes

pkg/dao/snapshots_test.go Show resolved Hide resolved

jlsherrill changed the title ~~Fixes 2702,Fixes 2887: Add orgID to snapshot list endpoint~~ Fixes 2702,2887: Add orgID to snapshot list endpoint Oct 31, 2023

Andrewgdewar force-pushed the HMS-2702 branch from 01de13b to 0abde92 Compare October 31, 2023 20:11

rverdile self-assigned this Nov 2, 2023

Andrewgdewar force-pushed the HMS-2702 branch from 0abde92 to 61ccda7 Compare November 3, 2023 16:37

Fixes 2702,2887: Add orgID to snapshot list endpoint

bd3b872

Andrewgdewar force-pushed the HMS-2702 branch from ec63892 to bd3b872 Compare November 6, 2023 20:26

rverdile reviewed Nov 7, 2023

View reviewed changes

Add command to snapshot a rhel repo

2d37d7b

jlsherrill force-pushed the HMS-2702 branch from 6eafadd to 2d37d7b Compare November 7, 2023 20:29

jlsherrill force-pushed the HMS-2702 branch from a5978cc to 2ed90e1 Compare November 8, 2023 13:17

Add debug to figure out timing issue

9318ada

jlsherrill force-pushed the HMS-2702 branch from 2ed90e1 to 9318ada Compare November 8, 2023 13:41

jlsherrill added 2 commits November 8, 2023 10:39

Revert "Add debug to figure out timing issue"

2681bc8

This reverts commit 9318ada.

adjust snapshot verbage

b0c1d03

Update error if is not found error

e341a78

Add test to ensure the 404 is returned

2d5321a

rverdile approved these changes Nov 10, 2023

View reviewed changes

jlsherrill added the Ready for QE label Nov 10, 2023

swadeley merged commit ab41987 into content-services:main Nov 13, 2023
7 checks passed

jlsherrill removed the Ready for QE label Nov 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes 2702,2887: Add orgID to snapshot list endpoint #435

Fixes 2702,2887: Add orgID to snapshot list endpoint #435

Andrewgdewar commented Oct 19, 2023 •

edited

Loading

jlsherrill commented Oct 19, 2023

jlsherrill commented Oct 25, 2023

jlsherrill commented Oct 31, 2023

rverdile commented Nov 3, 2023

swadeley commented Nov 6, 2023 •

edited

Loading

swadeley commented Nov 6, 2023

jlsherrill commented Nov 6, 2023

rverdile left a comment •

edited

Loading

rverdile Nov 7, 2023

Andrewgdewar Nov 10, 2023

jlsherrill commented Nov 7, 2023

jlsherrill commented Nov 7, 2023

jlsherrill commented Nov 7, 2023

swadeley commented Nov 8, 2023

jlsherrill commented Nov 8, 2023

jlsherrill commented Nov 8, 2023

swadeley commented Nov 9, 2023

swadeley commented Nov 10, 2023

Andrewgdewar commented Nov 10, 2023 •

edited

Loading

rverdile left a comment

Fixes 2702,2887: Add orgID to snapshot list endpoint #435

Fixes 2702,2887: Add orgID to snapshot list endpoint #435

Conversation

Andrewgdewar commented Oct 19, 2023 • edited Loading

Summary

Testing steps

jlsherrill commented Oct 19, 2023

jlsherrill commented Oct 25, 2023

jlsherrill commented Oct 31, 2023

rverdile commented Nov 3, 2023

swadeley commented Nov 6, 2023 • edited Loading

swadeley commented Nov 6, 2023

jlsherrill commented Nov 6, 2023

rverdile left a comment • edited Loading

Choose a reason for hiding this comment

rverdile Nov 7, 2023

Choose a reason for hiding this comment

Andrewgdewar Nov 10, 2023

Choose a reason for hiding this comment

jlsherrill commented Nov 7, 2023

jlsherrill commented Nov 7, 2023

jlsherrill commented Nov 7, 2023

swadeley commented Nov 8, 2023

jlsherrill commented Nov 8, 2023

jlsherrill commented Nov 8, 2023

swadeley commented Nov 9, 2023

swadeley commented Nov 10, 2023

Andrewgdewar commented Nov 10, 2023 • edited Loading

rverdile left a comment

Choose a reason for hiding this comment

Andrewgdewar commented Oct 19, 2023 •

edited

Loading

swadeley commented Nov 6, 2023 •

edited

Loading

rverdile left a comment •

edited

Loading

Andrewgdewar commented Nov 10, 2023 •

edited

Loading