-
Notifications
You must be signed in to change notification settings - Fork 284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #23319 - Add "latest" flag to katello/api/packages #7332
Conversation
Issues: #23319 |
'(CAST(katello_rpms.epoch AS INT) < CAST(katello_rpms2.epoch AS INT) OR ' + | ||
' (CAST(katello_rpms.epoch AS INT) = CAST(katello_rpms2.epoch AS INT) AND ' + | ||
' (katello_rpms.version_sortable < katello_rpms2.version_sortable OR ' + | ||
' (katello_rpms.version_sortable = katello_rpms2.version_sortable AND ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style/LineEndConcatenation: Use \ instead of + or << to concatenate those strings.
'katello_rpms.name = katello_rpms2.name AND katello_rpms.arch = katello_rpms2.arch AND ' + | ||
'(CAST(katello_rpms.epoch AS INT) < CAST(katello_rpms2.epoch AS INT) OR ' + | ||
' (CAST(katello_rpms.epoch AS INT) = CAST(katello_rpms2.epoch AS INT) AND ' + | ||
' (katello_rpms.version_sortable < katello_rpms2.version_sortable OR ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style/LineEndConcatenation: Use \ instead of + or << to concatenate those strings.
"LEFT OUTER JOIN (#{collection.to_sql}) AS katello_rpms2 ON " + | ||
'katello_rpms.name = katello_rpms2.name AND katello_rpms.arch = katello_rpms2.arch AND ' + | ||
'(CAST(katello_rpms.epoch AS INT) < CAST(katello_rpms2.epoch AS INT) OR ' + | ||
' (CAST(katello_rpms.epoch AS INT) = CAST(katello_rpms2.epoch AS INT) AND ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style/LineEndConcatenation: Use \ instead of + or << to concatenate those strings.
collection = collection.joins( | ||
"LEFT OUTER JOIN (#{collection.to_sql}) AS katello_rpms2 ON " + | ||
'katello_rpms.name = katello_rpms2.name AND katello_rpms.arch = katello_rpms2.arch AND ' + | ||
'(CAST(katello_rpms.epoch AS INT) < CAST(katello_rpms2.epoch AS INT) OR ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style/LineEndConcatenation: Use \ instead of + or << to concatenate those strings.
# aware of quirks like https://github.com/rails/rails/issues/18379 | ||
collection = collection.joins( | ||
"LEFT OUTER JOIN (#{collection.to_sql}) AS katello_rpms2 ON " + | ||
'katello_rpms.name = katello_rpms2.name AND katello_rpms.arch = katello_rpms2.arch AND ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style/LineEndConcatenation: Use \ instead of + or << to concatenate those strings.
# `collection.to_sql`, which may be somewhat brittle. For example, be | ||
# aware of quirks like https://github.com/rails/rails/issues/18379 | ||
collection = collection.joins( | ||
"LEFT OUTER JOIN (#{collection.to_sql}) AS katello_rpms2 ON " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style/LineEndConcatenation: Use \ instead of + or << to concatenate those strings.
[test katello] |
@jlsherrill Just making sure this PR is on your radar. |
ping |
' (katello_rpms.version_sortable COLLATE "C" < katello_rpms2.version_sortable COLLATE "C" OR ' \ | ||
' (katello_rpms.version_sortable = katello_rpms2.version_sortable AND ' \ | ||
' katello_rpms.release_sortable COLLATE "C" < katello_rpms2.release_sortable COLLATE "C"))))' | ||
).where('katello_rpms2.release_sortable IS NULL') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies again for the delay. Could we move this to the model? I think a 'latest' class method would make sense. It would look somethign like:
def self.latest
self.joins( "LEFT OUTER JOIN (#{self.to_sql}) ...........
end
Thats really where this belongs.
Also, what about the large amounts of comments below, planning on getting rid of those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can try moving this to the model.
As for the comments:
I selected the uncommented implementation based on the current behavior of Rails and the current performance characteristics of PostgreSQL using the current data set in my Satellite installation. It is entirely possible that one of the other commented implementations may work significantly better with a different version of Rails and/or with a different version PostgreSQL and/or with a different database and/or with a different data set.
Since the alternative implementations are significantly different from each other, since their performance characteristics are strongly influenced by unspecified implementation details in other pieces of software, and since they are not particularly obvious or intuitive solutions, I thought it would be a good idea to leave them in there as comments so a future developer could easily swap out the implementation if it becomes advantageous to do so.
I can remove them if you want, but we will loose knowledge about those possible alternatives, and someone may have to re-develop them if something changes and causes the uncommented implementation to behave or perform poorly.
I've moved However, note that
doesn't work and instead needs to be something like:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this review took so long. Thanks for this!
This PR was previously dependent on #7324, but #7324 has now been merged, and there are no further dependencies.