-
Notifications
You must be signed in to change notification settings - Fork 896
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
Fix extend_retires_on method for number of days. #14143
Conversation
@miq-bot add_label bug, euwe/yes |
The retires_on field was changed from a `:date` column to a `:datetime` which now requires the days(in seconds) be added to the time. - Depends on: ManageIQ/manageiq#14143 - This issue was introduced in PR ManageIQ/manageiq#11156 - https://bugzilla.redhat.com/show_bug.cgi?id=1427503 Signed-off-by: Sachin <psachin@redhat.com>
813bb73
to
938ae55
Compare
@mkanoor Please review. |
@@ -51,7 +51,7 @@ def retires_on=(timestamp) | |||
|
|||
def extend_retires_on(days, date = Time.zone.today) |
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.
@tinaafitz
Should the default value also be Time.zone.now
@@ -51,7 +51,7 @@ def retires_on=(timestamp) | |||
|
|||
def extend_retires_on(days, date = Time.zone.today) | |||
_log.info "Extending Retirement Date on #{self.class.name} id:<#{self.id}>, name:<#{self.name}> " | |||
new_retires_date = date + days.to_i | |||
new_retires_date = date + days.to_i.days |
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.
@tinaafitz
What is the date object coming into this function, would it have the time embedded in it? Since days returns a Duration object its possible that the time component could produce different results based on the timezone.
938ae55
to
72e364f
Compare
@mkanoor Please take another look. |
72e364f
to
6da714c
Compare
@mkanoor Added date validation. |
@@ -199,7 +199,7 @@ def invoke_ae | |||
:retirement_last_warn => Time.zone.today, | |||
:retirement_state => "retiring" | |||
) | |||
service_service.retires_on = Time.zone.today + 1 | |||
service_service.retires_on = Time.zone.today + 1.day |
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.
@tinaafitz
Should this be Time.zone.now?
6da714c
to
8359630
Compare
Checked commits tinaafitz/manageiq@ddb9238~...8359630 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@mkanoor Thanks for catching that. |
Fix extend_retires_on method for number of days. (cherry picked from commit f20cab8) https://bugzilla.redhat.com/show_bug.cgi?id=1433980
Euwe backport details:
|
The retires_on field was changed from a :date column to a :datetime
Issue brought to our attention by:
ManageIQ/manageiq-content#62
Prefer to use the model method extend_retires_on in Automate methods.
https://bugzilla.redhat.com/show_bug.cgi?id=1427503