Fix started_at/finished_at always being nil #496

agrare · 2023-06-23T16:09:37Z

The state_info had symbol keys like :startedAt and :finishedAt and we were looking for string keys like "startedAt" and "finishedAt" causing these values to always be nil even if data existed in the API response.

kbrock · 2023-06-23T19:56:02Z

@Fryguy Is it possible to raise the priority of this bugfix? Or are we thinking the deleted_on logic is not critical?

Fryguy · 2023-06-23T19:59:51Z

spec/models/manageiq/providers/kubernetes/container_manager/refresher_spec.rb

@@ -114,7 +114,7 @@ def assert_specific_container
          @container.last_started_at,
          @container.last_finished_at,
        ].each do |date_|
-          expect(date_.kind_of?(ActiveSupport::TimeWithZone) || date_.kind_of?(NilClass)).to be_truthy
+          expect(date_.kind_of?(ActiveSupport::TimeWithZone))


Was this change intentional? This expectation is no longer expecting anything?

Suggested change

expect(date_.kind_of?(ActiveSupport::TimeWithZone))

expect(date_.kind_of?(ActiveSupport::TimeWithZone)).to be

or

Suggested change

expect(date_.kind_of?(ActiveSupport::TimeWithZone))

expect(date_.kind_of?(ActiveSupport::TimeWithZone)).to be_truthy

I'm actually just going to drop the whole loop since expect time or nil is what hid this bug in the first place

In this VCR @container has started_at, last_started_at and last_finished_at set wit finished_at being nil so we can just check those directly

Fryguy · 2023-06-23T20:03:13Z

app/models/manageiq/providers/kubernetes/inventory/parser/container_manager.rb

@@ -853,10 +853,10 @@ def parse_container_state(state_hash)
    return {} if state_hash.to_h.empty?
    res = {}
    # state_hash key is the state and value are attributes e.g 'running': {...}
-    (state, state_info), = state_hash.to_h.to_a
+    (state, state_info), = state_hash.to_h.deep_symbolize_keys.to_a
    res[:state] = state
    %w(reason started_at finished_at exit_code signal message).each do |attr|


Maybe not for this PR, but we can pre-symbolize everything and not need to the .to_sym nor camelize calls calls - Something like:

Suggested change

%w(reason started_at finished_at exit_code signal message).each do |attr|

%i(reason reason started_at startedAt finished_at finishedAt exit_code exitCode signal signal message message).each_slice(2) do |attr, state_info_attr|

yea, instead of each_slice, just using a hash is easier for me to read. But both work

Fryguy · 2023-06-23T20:04:25Z

@kbrock we'll backport to petrosian as part of the set, yes. Even so, we'll need to deal with existing records anyway.

The `state_info` had symbol keys like `:startedAt` and `:finishedAt` and we were looking for string keys like `"startedAt"` and `"finishedAt"` causing these values to always be `nil` even if data existed in the API response.

miq-bot · 2023-07-11T16:21:28Z

Checked commit agrare@634e763 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🏆

kbrock · 2023-07-14T19:24:08Z

Rebase please. I think kubernetes HEAD fixes this issue

Fryguy · 2023-07-25T13:51:50Z

Backported to petrosian in commit 7793ce0.

commit 7793ce09194180ee171d172d73f9de22c95afdbb
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Fri Jul 14 16:09:47 2023 -0400

    Merge pull request #496 from agrare/fix_started_at_finished_at_timestamps
    
    Fix started_at/finished_at always being nil
    
    (cherry picked from commit 1c753fe979d1ecec638b211d88f9ff7af8d8ed92)

…amps Fix started_at/finished_at always being nil (cherry picked from commit 1c753fe)

agrare requested review from cben and Fryguy as code owners June 23, 2023 16:09

agrare assigned Fryguy Jun 23, 2023

agrare added bug petrosian/yes? labels Jun 23, 2023

kbrock approved these changes Jun 23, 2023

View reviewed changes

Fryguy reviewed Jun 23, 2023

View reviewed changes

agrare force-pushed the fix_started_at_finished_at_timestamps branch from f1fcc7a to 0d96172 Compare June 23, 2023 21:37

agrare force-pushed the fix_started_at_finished_at_timestamps branch from 0d96172 to 731d077 Compare July 11, 2023 16:11

Fix started_at/finished_at always being nil

634e763

The `state_info` had symbol keys like `:startedAt` and `:finishedAt` and we were looking for string keys like `"startedAt"` and `"finishedAt"` causing these values to always be `nil` even if data existed in the API response.

agrare force-pushed the fix_started_at_finished_at_timestamps branch from 731d077 to 634e763 Compare July 11, 2023 16:19

agrare closed this Jul 14, 2023

agrare reopened this Jul 14, 2023

kbrock assigned kbrock and unassigned Fryguy Jul 14, 2023

kbrock merged commit 1c753fe into ManageIQ:master Jul 14, 2023
3 of 5 checks passed

agrare deleted the fix_started_at_finished_at_timestamps branch July 14, 2023 20:13

Fryguy added petrosian/yes and removed petrosian/yes? labels Jul 19, 2023

Fryguy pushed a commit that referenced this pull request Jul 25, 2023

Merge pull request #496 from agrare/fix_started_at_finished_at_timest…

7793ce0

…amps Fix started_at/finished_at always being nil (cherry picked from commit 1c753fe)

Fryguy added petrosian/backported and removed petrosian/yes labels Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix started_at/finished_at always being nil #496

Fix started_at/finished_at always being nil #496

agrare commented Jun 23, 2023 •

edited

Loading

kbrock commented Jun 23, 2023

Fryguy Jun 23, 2023

agrare Jun 23, 2023 •

edited

Loading

Fryguy Jun 23, 2023 •

edited

Loading

kbrock Jul 11, 2023

agrare Jul 11, 2023

Fryguy commented Jun 23, 2023

miq-bot commented Jul 11, 2023

kbrock commented Jul 14, 2023

Fryguy commented Jul 25, 2023

	expect(date_.kind_of?(ActiveSupport::TimeWithZone))
	expect(date_.kind_of?(ActiveSupport::TimeWithZone)).to be

	%w(reason started_at finished_at exit_code signal message).each do \|attr\|
	%i(reason reason started_at startedAt finished_at finishedAt exit_code exitCode signal signal message message).each_slice(2) do \|attr, state_info_attr\|

Fix started_at/finished_at always being nil #496

Fix started_at/finished_at always being nil #496

Conversation

agrare commented Jun 23, 2023 • edited Loading

kbrock commented Jun 23, 2023

Fryguy Jun 23, 2023

Choose a reason for hiding this comment

agrare Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

Fryguy Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

kbrock Jul 11, 2023

Choose a reason for hiding this comment

agrare Jul 11, 2023

Choose a reason for hiding this comment

Fryguy commented Jun 23, 2023

miq-bot commented Jul 11, 2023

kbrock commented Jul 14, 2023

Fryguy commented Jul 25, 2023

agrare commented Jun 23, 2023 •

edited

Loading

agrare Jun 23, 2023 •

edited

Loading

Fryguy Jun 23, 2023 •

edited

Loading