-
Notifications
You must be signed in to change notification settings - Fork 987
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 #23704 - allow subnet order by network #5623
Conversation
Issues: #23704 |
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.
Nicely done, I like ignoring it for sqlite, that's sane approach. Small nitpick: I'd probably prefer using context in the test to prevent future copy-and-paste.
repushed with the context that deduplicates subnets setup, @lzap mind to take another look? |
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.
WFM
@@ -34,4 +34,19 @@ def subnet_params_filter | |||
def subnet_params | |||
self.class.subnet_params_filter.filter_params(params, parameter_filter_context) | |||
end | |||
|
|||
def network_reorder(base) |
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.
It feels like this should actually be a Model concern.
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.
I tried this approach, I failed to find a way to hook into scoped search, scoped search does not provide a way to implement casting, especially not for just some adapters. Then I thought let's do it in model, but I'd need to extend Subnet::ActiveRecord_Relation, not Subnet class. And then I gave up.
Now I realize that we can actually define it in Subnet model and it works just fine. So here you are, thanks for pushing me :-)
Subnet.all.network_reorder
Subnet.all.network_reorder 'network DESC'
3e62fcb
to
83e0ec0
Compare
test fails 💔 |
[test foreman] |
Relevant test failures. |
@ares this is a nice patch, can you rebase and make tests green? Then I am good to merge. |
I plan to revisit this after 1.24 branching when MySQL will be dropped. |
Dropped mysql and sqlite specific code, now focusing on postgres only. Also rebased, it should be ready fo rereview |
failures related |
I need to add sqlite switch back to silently ignore this :-/ |
this should now be green on sqlite too |
ready for rereview :-) |
Let's get this one in, it's opened for more than 1.5 year, pretty covered with tests, easy to test. Pretty please :-) |
ping @lzap |
def self.network_reorder(order_string = 'network') | ||
adapter = connection.adapter_name.downcase | ||
if adapter.starts_with?('postgresql') | ||
self.reorder(order_string.sub('network', 'inet(network)')) |
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.
Given we're now PostgreSQL-only, should we actually make a migration that uses the inet type for the column?
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.
don't we need to still support sqlite for ci?
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.
Yes, but you can have a conditional in the migration. You'd end up with the same difference that you have now. The only thing I'd be worried about is that it might serialize to a different object (IPAddr
for example) and the code would behave differently. That's why I won't insist on it, but would ask you to consider it.
@tbrisker what do you think?
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.
I haven't tested myself, but found this, meaning we'd get IPAddr
instance as you say. We'd need to update the method or many other places where .to_s
is not called implicitly, I guess API, templates could be affected
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.
Let's leave that till later, but long term I think it would be a good improvement. We can even drop the separate cidr field since PostgreSQL can store it in a single field.
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.
Thanks @ares !
Apologies for the delay, thanks indeed! |
No description provided.