Don't format PersistentVolume-capacity as bytes #17278

imtayadeway · 2018-04-10T19:14:15Z

There are other tables that have capacity columns that are formatted
as bytes. This newer column is serialized and will blow when trying to
apply this formatting to it. Since the existing way of mapping columns
to formats does not take table name into account, we need a temporary
way to override this for colliding column names. This can later be
reworked so all mapped formats will be qualified in some way.

Screens:

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1563861

NOTE: I did some refactoring to make this easier - probably best viewed as separate commits

There are other tables that have `capacity` columns that are formatted as bytes. This newer column is serialized and will blow when trying to apply this formatting to it. Since the existing way of mapping columns to formats does not take table name into account, we need a temporary way to override this for colliding column names. This can later be reworked so all mapped formats will be qualified in some way. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1563861

Fryguy · 2018-04-10T20:22:24Z

@imtayadeway Is this gaprindashvili/yes?

imtayadeway · 2018-04-10T20:32:06Z

@Fryguy ah yes, I believe it should be (corrected).

Fryguy · 2018-04-10T20:34:55Z

db/fixtures/miq_report_formats.yml

+  :formats_by_path:
+    :PersistentVolume-capacity: null
+    :ContainerVolumeKubernetes-capacity: null
+    :ContainerVolume-capacity: null


Minor, but this should be alphabetized.

Fryguy · 2018-04-10T20:35:14Z

app/models/miq_report/formats.rb

-    default_format_for(col, sfx, dt)
-  end
-
-  def self.default_format_for(column, suffix, datatype)


Is this not use anywhere else?

Doesn't look like it! I preferred to inline it rather than change the signature in this case

referenced only in manageiq proper:

app/models/miq_report/formats.rb
~~app/models/miq_report/formatting.rb~~
~~app/models/miq_report.rb~~
spec/models/miq_report/formats_spec.rb

updated -- those use default_format_for_path

@kbrock are you sure those are all hits, and not hitting default_format_for_path?

For reference (after my change):

tim@tim-thinkpad:~/src/manageiq$ ag "default_format_for" spec/models/miq_report/formats_spec.rb 20: describe '.default_format_for_path' do 32: subj = described_class.default_format_for_path("ChargebackVm-#{name}", datatype.first) 48: expect(described_class.default_format_for_path("ContainerVolume-capacity", :text)).to be_nil 49: expect(described_class.default_format_for_path("PersistentVolume-capacity", :text)).to be_nil 50: expect(described_class.default_format_for_path("ContainerVolumeKubernetes-capacity", :text)).to be_nil app/models/miq_report.rb 134: :default_format => Formats.default_format_for_path(path, data_type), app/models/miq_report/formatting.rb 15: format_name ||= MiqReport::Formats.default_format_for_path(col, nil) app/models/miq_report/formats.rb 22: def self.default_format_for_path(path, datatype) 36: format = FORMATS[default_format_for_path(path, datatype)]

No hits in the classic UI also, which is the only other place I might expect to find it

kbrock

Looks good to me. would like a second opinion.

kbrock · 2018-04-10T20:36:34Z

was asked in gitter about other "capacity" columns and data types:

column	datatype
Hardware.disk_capacity	bigint
MiqScsiLuns.capacity	bigint
PersistentVolumeClaim	text
ContainerVolume	text

…lumeClaim

gtanzillo · 2018-04-10T21:18:22Z

spec/models/miq_report/formats_spec.rb

@@ -43,5 +43,12 @@
        end
      end
    end
+


Since this spec is pretty small, perhaps it would be good to add another case that tests when there isn't the specific override. Just to be safe.

miq-bot · 2018-04-10T21:31:53Z

Checked commits imtayadeway/manageiq@8e4918c~...4c32131 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

cben · 2018-04-11T11:56:04Z

db/fixtures/miq_report_formats.yml

@@ -1,7 +1,10 @@
 ---
 :defaults_and_overrides:
  :formats_by_path:
+    :ContainerVolume-capacity: null
+    :ContainerVolumeKubernetes-capacity: null


just FYI , ContainerVolumeKubernetes subclass is completely unused.
(I tried to drop it, #15646, we concluded I should also do a migration but never got around to it)

…rt-format Don't format PersistentVolume-capacity as bytes (cherry picked from commit ad11173) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1566530

simaishi · 2018-04-12T13:21:32Z

Gaprindashvili backport details:

$ git log -1
commit 9dbcf7914405a091cd55a8c38a82200b677054d2
Author: Jason Frey <[email protected]>
Date:   Tue Apr 10 21:57:38 2018 -0400

    Merge pull request #17278 from imtayadeway/fix-pers-vol-capacity-report-format
    
    Don't format PersistentVolume-capacity as bytes
    (cherry picked from commit ad111733f385518df6e95773825955db4da38b9f)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1566530

imtayadeway added 3 commits April 10, 2018 11:09

Refactor test to use higher-level .default_format_for_path

8e4918c

Be more verbose when assigning temps

0ea8793

Inline default_format_for

9cd4819

imtayadeway added bug blocker labels Apr 10, 2018

imtayadeway assigned Fryguy Apr 10, 2018

imtayadeway requested a review from gtanzillo April 10, 2018 19:20

imtayadeway force-pushed the fix-pers-vol-capacity-report-format branch from 59d6a73 to 32c9f02 Compare April 10, 2018 19:34

imtayadeway added the gaprindashvili/yes label Apr 10, 2018

Fryguy reviewed Apr 10, 2018

View reviewed changes

kbrock approved these changes Apr 10, 2018

View reviewed changes

imtayadeway force-pushed the fix-pers-vol-capacity-report-format branch from 32c9f02 to 9a33943 Compare April 10, 2018 20:39

Fix capacity collision for ContainerVolume{,Kubernetes}, PersistentVo…

c858e35

…lumeClaim

imtayadeway force-pushed the fix-pers-vol-capacity-report-format branch from 9a33943 to c858e35 Compare April 10, 2018 20:48

gtanzillo reviewed Apr 10, 2018

View reviewed changes

Ensure that the default case for capacity still works

4c32131

gtanzillo approved these changes Apr 10, 2018

View reviewed changes

Fryguy merged commit ad11173 into ManageIQ:master Apr 11, 2018

Fryguy added this to the Sprint 84 Ending Apr 23, 2018 milestone Apr 11, 2018

cben reviewed Apr 11, 2018

View reviewed changes

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Apr 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't format PersistentVolume-capacity as bytes #17278

Don't format PersistentVolume-capacity as bytes #17278

imtayadeway commented Apr 10, 2018

Fryguy commented Apr 10, 2018

imtayadeway commented Apr 10, 2018

Fryguy Apr 10, 2018

Fryguy Apr 10, 2018

imtayadeway Apr 10, 2018

kbrock Apr 10, 2018 •

edited

Loading

imtayadeway Apr 10, 2018

imtayadeway Apr 10, 2018

imtayadeway Apr 10, 2018

kbrock left a comment

kbrock commented Apr 10, 2018 •

edited

Loading

gtanzillo Apr 10, 2018

miq-bot commented Apr 10, 2018

cben Apr 11, 2018

simaishi commented Apr 12, 2018

@@ @@ -43,5 +43,12 @@ @@
                       end
                     end
                   end

Don't format PersistentVolume-capacity as bytes #17278

Don't format PersistentVolume-capacity as bytes #17278

Conversation

imtayadeway commented Apr 10, 2018

Fryguy commented Apr 10, 2018

imtayadeway commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Apr 10, 2018 • edited Loading

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

kbrock left a comment

Choose a reason for hiding this comment

kbrock commented Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

miq-bot commented Apr 10, 2018

Choose a reason for hiding this comment

simaishi commented Apr 12, 2018

kbrock Apr 10, 2018 •

edited

Loading

kbrock commented Apr 10, 2018 •

edited

Loading