Fixes #15630: Add file content type API #6176
Conversation
|
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
|
Will require #5770 |
|
|
||
| def update_from_json(json) | ||
| keys = %w(name checksum) | ||
| custom_json = json.clone.delete_if { |key, _value| !keys.include?(key) } |
There was a problem hiding this comment.
Seems weird to be using clone and delete_if instead of just reject. Better yet, just use slice:
custom_json = json.slice('name', 'checksum')There was a problem hiding this comment.
Sorry I missed this -- do you think this changed should be made everywhere? Given every repository type implements this function?
There was a problem hiding this comment.
I think just here is fine. Unless you feel the need to update it everywhere. Conversely, if you want to keep this the way it is to be consistent with other repository types, that's fine too. Your call.
|
Rebased on top of the recently merged file PR |
|
This still needs some tests added to it but if someone wants to review it for the basic design and data model that would be greatly appreciated. |
|
getting this when syncing: |
|
Error when indexing content Looks like pulp_database_unit#sync_repository_associations is expecting the class name to match the attribute name on the join relation. |
jlsherrill
added
Waiting on contributor
and removed
Needs testing
Not yet reviewed
labels
theforeman-bot
added
Needs testing
Needs re-review
and removed
Waiting on contributor
labels
ehelms
force-pushed
the
fixes-15630
branch
2 times, most recently
from
a4edb95
to
38c982d
Compare
|
@jlsherrill updated |
|
What about this comment: #6176 (comment) |
|
@daviddavis addressed |
| t.timestamps | ||
| t.string "uuid", :null => false, :limit => 255 | ||
| t.string 'name', :limit => 255 | ||
| t.string 'checksum', :limit => 255 |
There was a problem hiding this comment.
Does name and checksum really need to be limited to 255?
There was a problem hiding this comment.
I was following the other content types that were set to this based on (#5837). If you think I should open this up to unlimited then I shall.
There was a problem hiding this comment.
@jlsherrill locally I have it looking like this:
{
id: 6,
name: "katello-ca-consumer-latest.noarch.rpm",
path: "pops/hey/WHAT",
uuid: "9d03acff-e674-4f49-8753-f900e7d22d12",
checksum: "24065bf79b81abd806fbe989edcfffcc1e3b4bb8a02726cbfbc957de3df166fd"
},
Do you think the full path would be better there? or just let users and interfaces construct it?
There was a problem hiding this comment.
I might would just have the full path and not the partial path. That seems simpler and the user can do with it what they want.
name: "katello-ca-consumer-latest.noarch.rpm",
path: "pops/hey/WHAT/katello-ca-consumer-latest.noarch.rpm"
|
Updated:
|
|
https://github.com/Katello/katello/blob/master/app/controllers/katello/concerns/api/v2/repository_content_controller.rb#L187-L204 needs to be updated otherwise requests for nonexistant files throws an error: GET http://robot:3000/katello/api/v2/files/zoom "Can't find resource class: Katello::FileUnit" |
This also adds importing of file type data into the database as that is needed in ordered to display the API information.
|
@jlsherrill Good catch -- updated |
|
APJ |
|
[test] |
No description provided.