Bug #13217

Discovery Rules order by name, not priority

Added by Steve Parker about 1 year ago. Updated about 1 year ago.

Status:Closed
Priority:Urgent
Assigned To:Lukas Zapletal
Category:Image
Target version:Plugin 5.0.3
Difficulty: Pull request:https://github.com/theforeman/foreman_discovery/pull/254
Bugzilla link:1320492
Story points-
Velocity based estimate-

Description

Discovery rules are processed in order of :name, not :priority.

A simple fix is in line 9 of foreman_discovery-4.0.0/app/controllers/concerns/foreman/controller/discovered_extensions.rb:

-    DiscoveryRule.where(:enabled => true).reorder(:priority).each do |rule|
+    DiscoveryRule.where(:enabled => true).order(:priority).each do |rule|

I don't know for sure, but think it could be related to foreman_discovery-4.0.0/app/models/discovery_rule.rb, which sets: order("discover_rules.name") around line 29:

  # with proc support, default_scope can no longer be chained
  # include all default scoping here
  default_scope lambda {
                  with_taxonomy_scope do
                    order("discovery_rules.name")
                  end
                }


Related issues

Related to Discovery - Bug #12765: Inconsistent rule ordering in the UI Closed 12/10/2015

Associated revisions

Revision b1d56894
Added by Lukas Zapletal about 1 year ago

Fixes #13217 - rule ordering fixed

History

#1 Updated by Steve Parker about 1 year ago

My apologies, reverse diff format in the original report. The fix is to use ".reorder" instead of ".order"

#2 Updated by Lukas Zapletal about 1 year ago

  • Category set to Image
  • Assigned To set to Lukas Zapletal
  • Priority changed from Normal to Urgent
  • Target version set to Plugin 6.0

Oh thanks!

#3 Updated by Lukas Zapletal about 1 year ago

  • Related to Bug #12765: Inconsistent rule ordering in the UI added

#4 Updated by Lukas Zapletal about 1 year ago

  • Target version changed from Plugin 6.0 to Plugin 5.0.2

What a nasty bug, Steve. Thanks for investigation!

#5 Updated by Lukas Zapletal about 1 year ago

For the record, I am clearing our documentation which was also a bit fuzzy around this: https://github.com/theforeman/theforeman.org/pull/556

#6 Updated by The Foreman Bot about 1 year ago

  • Status changed from New to Ready For Testing
  • Pull request https://github.com/theforeman/foreman_discovery/pull/254 added

#7 Updated by Lukas Zapletal about 1 year ago

  • Target version changed from Plugin 5.0.2 to Plugin 5.0.3

#8 Updated by Anonymous about 1 year ago

  • Status changed from Ready For Testing to Closed
  • % Done changed from 0 to 100

#9 Updated by Lukas Zapletal about 1 year ago

  • Bugzilla link set to 1320492

Also available in: Atom PDF