Fixes 4293: add support for upload only repos #728

jlsherrill · 2024-07-02T13:15:17Z

Summary

Testing steps

Create an upload repo:

PATCH http://localhost:8000/api/content-sources/v1.0/repositories/c06a0512-0954-4b11-9b13-3358afe77f46
x-rh-identity: eyJpZGVudGl0eSI6eyJvcmdfaWQiOiI5IiwgInR5cGUiOiJVc2VyIiwidXNlciI6eyJ1c2VybmFtZSI6ImZvbyJ9LCJhY2NvdW50X251bWJlciI6ImZvbyIsImludGVybmFsIjp7Im9yZ19pZCI6IjkifX19Cg==
//x-Rh-Insights-Request-Id: 9d229d34-7219-405d-ba3d-8f99cf7c36ae
Content-Type: application/json

{
  "name": "arg",
  "origin": "upload"
}

with batch:

POST http://localhost:8000/api/content-sources/v1.0/repositories/bulk_create/
x-rh-identity: eyJpZGVudGl0eSI6eyJvcmdfaWQiOiI5IiwgInR5cGUiOiJVc2VyIiwidXNlciI6eyJ1c2VybmFtZSI6ImZvbyJ9LCJhY2NvdW50X251bWJlciI6ImZvbyIsImludGVybmFsIjp7Im9yZ19pZCI6IjkifX19Cg==

[{
  "name": "test5",
  "gpg_key": "",
  "snapshot": true,
  "origin": "upload",
  "url": ""
},
  {
    "name": "test6",
    "gpg_key": "",
    "snapshot": true,
    "origin": "upload",
    "url": ""
  }]

try to:

create/update an upload repo with a url, this should fail
try to change the origin of a repo, this should not succeed
try to introspect an upload repo, this should be rejected
try to snapshot an upload repo, this should be rejected
run go run cmd/external-repos/main.go nightly-jobs and verify no errors, that things work as expected

Checklist

Tested with snapshotting feature disabled and pulp server URL not configured if appropriate

jlsherrill · 2024-07-02T13:30:13Z

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

db/migrations/20240627171711_allow_null_url.down.sql

pkg/handler/repositories.go

rverdile · 2024-07-08T18:00:30Z

pkg/api/repositories.go

+	Snapshot             *bool     `json:"snapshot"`                            // Enable snapshotting and hosting of this repository
+}
+
+type RepositoryUpdateRequest struct {


Not saying I'm opposed, just curious, why separate the update request struct? I guess one benefit might be making the API more readable.

While making the origin parameter available for create, i didn't want it available for update. There's no way in openapi to indicate that a parameter is only for creation, and i saw some discussion mentioning that using different structs was the correct way to handle it. We also did it this way for template.

That said, it complicates and duplicates the code quite a bit. I tried using inheritance and that reduced the duplication a bit, but increased complexity and i didn't think was an improvement.

rverdile

I found these two issues. Everything else seems to be working well.

rverdile · 2024-07-09T17:30:04Z

pkg/dao/repository_configs.go

+	} else if newRepo.URL != "" {
+		cleanedUrl := models.CleanupURL(newRepo.URL)
+		// Repo configs with the same URL share a repository object
+		if err := r.db.WithContext(ctx).Where("url = ?", cleanedUrl).FirstOrCreate(&newRepo).Error; err != nil {
+			return api.RepositoryResponse{}, DBErrorToApi(err)
+		}


If a repository exists (not a repository config) and I create an upload repo with the URL of the existing repository, I get no error, because it fetches the existing repository (which would have a non-upload origin). If the repository does not exist, I get a 400 error because "an upload repo cannot contain a URL". I think I'd expect to get the same 400 in the first scenario.

updated and add a test for create and update for this situation

rverdile · 2024-07-09T17:31:36Z

db/migrations/20240627171711_allow_null_url.down.sql

+BEGIN;
+alter table repositories alter column URL set not null;
+COMMIT;


I get this error running the down migration

{"level":"error","error":"2 errors occurred:\n\t* migration failed: column \"url\" of relation \"repositories\" contains null values in line 0: BEGIN;\nalter table repositories alter column URL set not null;\nCOMMIT;\n (details: pq: column \"url\" of relation \"repositories\" contains null values)\n\t* pq: current transaction is aborted, commands ignored until end of transaction block in line 0: SELECT pg_advisory_unlock($1)\n\n","time":"2024-07-09T13:29:07-04:00","message":"Failed to migrate:"} {"level":"fatal","error":"database locked","time":"2024-07-09T13:29:07-04:00","message":"Failed to migrate"}

I had an upload repository created when I ran it

so i'm not sure exactly what to do about this. I guess this may be considered a migration that can't be reverted. We can't flip that column back to 'not null' whith a null value. We could delete all repositories (and repo configs, and associated rpms, etc....) in the migration, but thats not a great solution. Thoughts?

That's true. I can't think of a better way to do it. I guess we can consider it an un-revertable migration.

rverdile

looks good!

swadeley · 2024-07-15T08:43:30Z

/retest

swadeley · 2024-07-15T15:06:39Z

Hi

I can create an upload repo:


In [7]:     repo_dict_3 = dict(
   ...:         name="test_upload_repo_2",
   ...:         origin="upload",
   ...:         snapshot=True,
   ...:      )

In [8]: created_repo3 = app.content_sources.rest_client.repositories_api.create_repository(repo_dict_3)
2024-07-15 15:10:58.648 [    INFO] [iqe.base.rest_client] REST: METHOD=POST, request_id=<>, params=[]

In [9]: created_repo3
Out[9]: 
{'account_id': '0369233',
 'content_type': 'rpm',
 'distribution_arch': 'any',
 'distribution_versions': ['any'],
 'failed_introspections_count': 0,
 'gpg_key': '',
 'label': 'test_upload_repo_2',
 'last_introspection_error': '',
 'last_introspection_status': 'Pending',
 'last_introspection_time': '',
 'last_success_introspection_time': '',
 'last_update_introspection_time': '',
 'metadata_verification': False,
 'module_hotfixes': False,
 'name': 'test_upload_repo_2',
 'org_id': '3340851',
 'origin': 'upload',
 'package_count': 0,
 'snapshot': True,
 'status': 'Pending',
 'url': '',
 'uuid': 'be85f598-62a2-427a-bfaa-e14becd4b4c0'}

To list upload repos at present it is required to use origin='upload':

In [20]: app.content_sources.rest_client.repositories_api.list_repositories(origin='upload')
2024-07-15 16:00:51.627 [    INFO] [iqe.base.rest_client] REST: METHOD=GET, request_id=<>, params=[('origin', 'upload')]
Out[20]: 
{'data': [{'account_id': '0369233',
           'content_type': 'rpm',
           'distribution_arch': 'any',
           'distribution_versions': ['any'],
           'failed_introspections_count': 0,
           'gpg_key': '',
           'label': 'test_upload_repo_1',
           'last_introspection_error': '',
           'last_introspection_status': 'Pending',
           'last_introspection_time': '',
           'last_success_introspection_time': '',
           'last_update_introspection_time': '',
           'metadata_verification': False,
           'module_hotfixes': False,
           'name': 'test_upload_repo_1',
           'org_id': '3340851',
           'origin': 'upload',
           'package_count': 0,
           'snapshot': False,
           'status': 'Pending',
           'url': '',
           'uuid': 'e1474a4a-c4ba-48d8-8bbf-9b513d240711'},
          {'account_id': '0369233',
           'content_type': 'rpm',
           'distribution_arch': 'any',
           'distribution_versions': ['any'],
           'failed_introspections_count': 0,
           'gpg_key': '',
           'label': 'test_upload_repo_2',
           'last_introspection_error': '',
           'last_introspection_status': 'Pending',
           'last_introspection_time': '',
           'last_success_introspection_time': '',
           'last_update_introspection_time': '',
           'metadata_verification': False,
           'module_hotfixes': False,
           'name': 'test_upload_repo_3',
           'org_id': '3340851',
           'origin': 'upload',
           'package_count': 0,
           'snapshot': True,
           'status': 'Pending',
           'url': '',
           'uuid': 'be85f598-62a2-427a-bfaa-e14becd4b4c0'}],
 'links': {'first': '/api/content-sources/v1/repositories/?limit=100&offset=0&origin=upload',
           'last': '/api/content-sources/v1/repositories/?limit=100&offset=0&origin=upload'},
 'meta': {'count': 2, 'limit': 100, 'offset': 0}}

In [21]:

jlsherrill added the qe-testing-needed label Jul 2, 2024

jlsherrill force-pushed the 4293 branch from 87aac1b to 93313ef Compare July 5, 2024 14:30

jlsherrill marked this pull request as ready for review July 5, 2024 14:38

rverdile self-assigned this Jul 8, 2024

jlsherrill commented Jul 8, 2024

View reviewed changes

db/migrations/20240627171711_allow_null_url.down.sql Outdated Show resolved Hide resolved

jlsherrill force-pushed the 4293 branch from 93313ef to 6d264f2 Compare July 8, 2024 17:22

rverdile reviewed Jul 8, 2024

View reviewed changes

pkg/handler/repositories.go Outdated Show resolved Hide resolved

rverdile reviewed Jul 8, 2024

View reviewed changes

pkg/handler/repositories.go Outdated Show resolved Hide resolved

rverdile reviewed Jul 8, 2024

View reviewed changes

jlsherrill mentioned this pull request Jul 9, 2024

Fixes 4294: snapshot on create for upload repo #732

Merged

1 task

rverdile reviewed Jul 9, 2024

View reviewed changes

jlsherrill added 3 commits July 11, 2024 15:38

Fixes 4293: add support for upload only repos

f1dea67

address comments

0b6bead

Reject create/update with a URL for upload repos

f58b96d

jlsherrill force-pushed the 4293 branch from f51a3df to f58b96d Compare July 11, 2024 21:10

rverdile approved these changes Jul 12, 2024

View reviewed changes

jlsherrill added the Ready for QE label Jul 12, 2024

swadeley merged commit 8873452 into content-services:main Jul 15, 2024
8 checks passed

jlsherrill removed the Ready for QE label Jul 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes 4293: add support for upload only repos #728

Fixes 4293: add support for upload only repos #728

jlsherrill commented Jul 2, 2024 •

edited

Loading

jlsherrill commented Jul 2, 2024

rverdile Jul 8, 2024

jlsherrill Jul 8, 2024

rverdile left a comment

rverdile Jul 9, 2024

jlsherrill Jul 12, 2024

rverdile Jul 9, 2024

rverdile Jul 9, 2024

jlsherrill Jul 10, 2024

rverdile Jul 10, 2024

rverdile left a comment

swadeley commented Jul 15, 2024

swadeley commented Jul 15, 2024

Fixes 4293: add support for upload only repos #728

Fixes 4293: add support for upload only repos #728

Conversation

jlsherrill commented Jul 2, 2024 • edited Loading

Summary

Testing steps

Checklist

jlsherrill commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rverdile left a 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

rverdile left a comment

Choose a reason for hiding this comment

swadeley commented Jul 15, 2024

swadeley commented Jul 15, 2024

jlsherrill commented Jul 2, 2024 •

edited

Loading