Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix: do not raise error if schedule for widget exists but not linked #19037

Merged
merged 3 commits into from
Aug 8, 2019
Merged

Fix: do not raise error if schedule for widget exists but not linked #19037

merged 3 commits into from
Aug 8, 2019

Conversation

yrudman
Copy link
Contributor

@yrudman yrudman commented Jul 23, 2019

It would resolve issue with failing seeding in case when schedule exists but not linked to corresponding widget.

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

@miq-bot add-label bug, core, hammer/yes

\cc @gtanzillo

@yrudman
Copy link
Contributor Author

yrudman commented Jul 23, 2019

@miq-bot add-label ivanchuk/yes

@miq-bot
Copy link
Member

miq-bot commented Jul 23, 2019

@yrudman Cannot apply the following label because they are not recognized: ivanchuk/yes

bdunne
bdunne reviewed Jul 23, 2019
View reviewed changes
app/models/miq_widget.rb Outdated Show resolved Hide resolved
@yrudman yrudman force-pushed the che-for-existted-sheduler-when-seeding branch from 0f7a0a1 to 48128a2 Compare July 24, 2019 15:13
@yrudman
Copy link
Contributor Author

yrudman commented Jul 24, 2019

@miq-bot add-label ivanchuk/yes

gtanzillo
gtanzillo approved these changes Jul 25, 2019
View reviewed changes
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

kbrock
kbrock reviewed Jul 30, 2019
View reviewed changes
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from a code perspective this looks close.

I don't understand the reasoning... but am ok with merging.
Is there someone who understands the reasoning for this?

spec/models/miq_widget_spec.rb Outdated Show resolved Hide resolved
app/models/miq_widget.rb Show resolved Hide resolved
@yrudman
Copy link
Contributor Author

yrudman commented Jul 30, 2019

@kbrock you already found root cause of issue - #18489.
I am not sure if we need to go forward with this PR ...

@yrudman
Copy link
Contributor Author

yrudman commented Jul 30, 2019

found #18489 when started looking at another BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1734076 - failing validation on the same report name instead of widget

@yrudman yrudman force-pushed the che-for-existted-sheduler-when-seeding branch from 26c4191 to 4f2478b Compare July 30, 2019 17:55
@yrudman
Copy link
Contributor Author

yrudman commented Jul 30, 2019

@kbrock @gtanzillo on the second thought, - may be we should merge this PR, it brings no harm ....

@kbrock
Copy link
Member

kbrock commented Jul 30, 2019

So there are duplicate reports. And both of them got scheduled. So we have duplicate schedules.

Not sure how modifying the schedules so they look different fixes our underlying issue: a report that is deprecated was not removed.

(Or maybe the root cause is a different issue, and I'm confused)

@yrudman
Copy link
Contributor Author

yrudman commented Jul 30, 2019

I am confused to.
There are no duplicated reports in client's db
I am not sure how it happened, but the only issue on client's db is - schedule with name of "Pods per Ready Status Chart" exists, but miq_widget#miq_schedule_id is nil for widget with the same title , and seeding failing when trying to create schedule for that widget

Fryguy
Fryguy reviewed Jul 30, 2019
View reviewed changes
app/models/miq_widget.rb Outdated Show resolved Hide resolved
@yrudman yrudman force-pushed the che-for-existted-sheduler-when-seeding branch from 3186fb5 to 7d3c152 Compare July 30, 2019 20:47
@yrudman
Copy link
Contributor Author

yrudman commented Jul 31, 2019

@miq-bot add-label changelog/yes

@yrudman
Copy link
Contributor Author

yrudman commented Aug 7, 2019

@gtanzillo @kbrock @bdunne can this be merged if there are no other objections ?

gtanzillo
gtanzillo approved these changes Aug 7, 2019
View reviewed changes
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

bdunne
bdunne reviewed Aug 7, 2019
View reviewed changes
app/models/miq_widget.rb Outdated Show resolved Hide resolved
bdunne
bdunne reviewed Aug 7, 2019
View reviewed changes
app/models/miq_widget.rb Outdated Show resolved Hide resolved
bdunne
bdunne reviewed Aug 7, 2019
View reviewed changes
app/models/miq_widget.rb Outdated Show resolved Hide resolved
bdunne
bdunne reviewed Aug 7, 2019
View reviewed changes
app/models/miq_widget.rb Outdated Show resolved Hide resolved
@yrudman
fix: check if shedule for widget with the same name already exists an…
484c881 
…d reuse it instead of raising error. It would resolve issue with failing seeding in case when chedule exists but not libked to corresponding widget.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1729441
@yrudman yrudman force-pushed the che-for-existted-sheduler-when-seeding branch from 117f935 to ef41d44 Compare August 7, 2019 20:27
@yrudman yrudman force-pushed the che-for-existted-sheduler-when-seeding branch from ef41d44 to 5384b1c Compare August 7, 2019 21:19
@miq-bot
Copy link
Member

miq-bot commented Aug 7, 2019

Checked commits yrudman/manageiq@484c881~...5384b1c with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@bdunne bdunne merged commit a248ba3 into ManageIQ:master Aug 8, 2019
@bdunne bdunne self-assigned this Aug 8, 2019
@bdunne bdunne added this to the Sprint 118 Ending Aug 19, 2019 milestone Aug 8, 2019
@yrudman yrudman deleted the che-for-existted-sheduler-when-seeding branch August 8, 2019 13:56
simaishi pushed a commit that referenced this pull request Aug 8, 2019
@bdunne @simaishi
Merge pull request #19037 from yrudman/che-for-existted-sheduler-when…
9a91d36 
…-seeding

Fix: do not raise error if schedule for widget exists but not linked
(cherry picked from commit a248ba3)

https://bugzilla.redhat.com/show_bug.cgi?id=1729441
@simaishi
Copy link
Contributor

simaishi commented Aug 8, 2019

Ivanchuk backport details:

$ git log -1
commit 9a91d360a914c8c34baa321406691b3929a986b7
Author: Brandon Dunne <[email protected]>
Date:   Thu Aug 8 09:43:12 2019 -0400

    Merge pull request #19037 from yrudman/che-for-existted-sheduler-when-seeding
    
    Fix: do not raise error if schedule for widget exists but not linked
    (cherry picked from commit a248ba302ef069a25d4967eee97f363d41d35745)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1729441

simaishi pushed a commit that referenced this pull request Aug 14, 2019
@bdunne @simaishi
Merge pull request #19037 from yrudman/che-for-existted-sheduler-when…
e48bbd7 
…-seeding

Fix: do not raise error if schedule for widget exists but not linked
(cherry picked from commit a248ba3)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1740229
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit e48bbd758e9b9f6e914d9cff9e66a9695ca79f13
Author: Brandon Dunne <[email protected]>
Date:   Thu Aug 8 09:43:12 2019 -0400

    Merge pull request #19037 from yrudman/che-for-existted-sheduler-when-seeding
    
    Fix: do not raise error if schedule for widget exists but not linked
    (cherry picked from commit a248ba302ef069a25d4967eee97f363d41d35745)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1740229

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdunne bdunne bdunne left review comments

@gtanzillo gtanzillo gtanzillo approved these changes

@Fryguy Fryguy Fryguy left review comments

@kbrock kbrock kbrock left review comments

Assignees

@bdunne bdunne

Labels
bug changelog/yes core hammer/backported ivanchuk/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@yrudman @miq-bot @kbrock @simaishi @Fryguy @gtanzillo @bdunne