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

Successive automation task runs shouldn't change the original params #19158

Merged
merged 1 commit into from
Aug 22, 2019
Merged

Successive automation task runs shouldn't change the original params #19158

merged 1 commit into from
Aug 22, 2019

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Aug 15, 2019 •
edited
Loading

Params get removed and re-added here: https://github.com/ManageIQ/manageiq/pull/12722/files#diff-e3295c427cec18ad642950055ae49a3bR53 and on the second run of an automation task that's scheduled, we've lost the relevant attributes.

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

@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 15, 2019

@miq-bot add_label bug, hammer/yes, ivanchuk/yes
@miq-bot assign @tinaafitz

@miq-bot miq-bot added the wip label Aug 15, 2019
@d-m-u d-m-u changed the title [wip] successive runs shouldn't change the original params [wip] successive automation task runs shouldn't change the original params Aug 19, 2019
@d-m-u d-m-u force-pushed the successive_automation_task_runs_should_not_remove_params branch from 8480ad1 to ae4cc6b Compare August 20, 2019 17:56
@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 20, 2019

@bdunne could I get you to review, please?

@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 20, 2019

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [wip] successive automation task runs shouldn't change the original params successive automation task runs shouldn't change the original params Aug 20, 2019
@miq-bot miq-bot removed the wip label Aug 20, 2019
bdunne
bdunne reviewed Aug 20, 2019
View reviewed changes
Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

I'm trying to better understand the problem...

  • I see that the linked change modifies filter[:parameters], but is that a problem? Is the changed record saved? Wouldn't the record be reloaded on the next schedule run?
  • Should AutomationRequest#parse_out_objects modify the input? It seems to me like It should not modify inputs and instead return a duplicate, but I don't really know. Maybe @syncrou or @gmcculloug can help?

@gmcculloug
Copy link
Member

@bdunne Yeah, I can see moving this change down into AutomateRequest class. Taking a closer look at the app/models/automation_request.rb there are a few calls to .delete that we should be protecting against. This was a first shot at seeing if we identified the root cause of the issue. The test still seems valid.

@d-m-u Let me know if you want to work through this together.

@d-m-u d-m-u changed the title successive automation task runs shouldn't change the original params [wip] successive automation task runs shouldn't change the original params Aug 20, 2019
@miq-bot miq-bot added the wip label Aug 20, 2019
@d-m-u d-m-u force-pushed the successive_automation_task_runs_should_not_remove_params branch 3 times, most recently from beb22e5 to e6a6e4a Compare August 21, 2019 13:25
@gmcculloug gmcculloug requested a review from syncrou August 21, 2019 13:41
@gmcculloug
Copy link
Member

@syncrou I worked with @d-m-u on these changes so would not mind you taking a look.

gmcculloug
gmcculloug reviewed Aug 21, 2019
View reviewed changes
app/models/automation_request.rb
parameters.stringify_keys!
create_from_ws("1.1", user, uri_parts, parameters, approval)
uri_parts = uri_parts.stringify_keys
parameters = parameters.stringify_keys
Copy link
Member

Choose a reason for hiding this comment

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

By using stringify_keys without ! we are dup'ing the objects at the top of the method so uri_parts.delete that was happening on the first line of the method before will no longer modify the object passed in. 👍

@d-m-u d-m-u force-pushed the successive_automation_task_runs_should_not_remove_params branch from e6a6e4a to 9bb523e Compare August 21, 2019 13:52
@d-m-u d-m-u changed the title [wip] successive automation task runs shouldn't change the original params Successive automation task runs shouldn't change the original params Aug 21, 2019
@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 21, 2019

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Aug 21, 2019
@d-m-u d-m-u force-pushed the successive_automation_task_runs_should_not_remove_params branch 3 times, most recently from c769426 to 92b4bc2 Compare August 21, 2019 14:45
@d-m-u d-m-u force-pushed the successive_automation_task_runs_should_not_remove_params branch from 4c412a6 to 20b37fc Compare August 21, 2019 17:14
bdunne
bdunne reviewed Aug 21, 2019
View reviewed changes
app/models/automation_request.rb Outdated
object_hash.each do |key, _v|
parameters.delete(key)
end
uri_parts = uri_parts.stringify_keys
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of uri_parts.except(:namespace, :class_name).stringify_keys!?

@d-m-u d-m-u force-pushed the successive_automation_task_runs_should_not_remove_params branch from 20b37fc to b80853b Compare August 21, 2019 17:30
bdunne
bdunne reviewed Aug 21, 2019
View reviewed changes
app/models/automation_request.rb Outdated
object_hash.each do |key, _v|
parameters.delete(key)
end
uri_parts = uri_parts.except(:namespace, :class_name).stringify_keys
Copy link
Member

Choose a reason for hiding this comment

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

In this case we can use stringify_keys! because except is already creating a new object.

Copy link
Contributor Author

@d-m-u d-m-u Aug 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah okay, fine fine.

@gmcculloug gmcculloug assigned gmcculloug and unassigned tinaafitz Aug 21, 2019
@d-m-u d-m-u force-pushed the successive_automation_task_runs_should_not_remove_params branch from b80853b to 047b695 Compare August 21, 2019 19:16
@bdunne
Copy link
Member

bdunne commented Aug 21, 2019

@d-m-u It looks like something got reverted back to the [].each { |i| x.delete(i)... pattern.

@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 21, 2019

@bdunne yeah, I can't look into the spec failure with your suggestion #19158 (comment) just right now, so it's back to when it worked, will get to it tomorrow

@d-m-u d-m-u force-pushed the successive_automation_task_runs_should_not_remove_params branch from 047b695 to f89526d Compare August 21, 2019 21:23
@miq-bot
Copy link
Member

miq-bot commented Aug 21, 2019

Checked commit d-m-u@f89526d with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

@gmcculloug
Copy link
Member

Tests are passing. @bdunne PTAL

@bdunne bdunne merged commit c86ed36 into ManageIQ:master Aug 22, 2019
@bdunne bdunne added this to the Sprint 119 Ending Sep 2, 2019 milestone Aug 22, 2019
@bdunne bdunne assigned bdunne and unassigned gmcculloug Aug 22, 2019
simaishi pushed a commit that referenced this pull request Aug 23, 2019
@bdunne @simaishi
Merge pull request #19158 from d-m-u/successive_automation_task_runs_…
c4ebda3 
…should_not_remove_params

Successive automation task runs shouldn't change the original params

(cherry picked from commit c86ed36)

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

Ivanchuk backport details:

$ git log -1
commit c4ebda3585b920ffd332d32d367382b78ac3f007
Author: Brandon Dunne <[email protected]>
Date:   Thu Aug 22 08:16:53 2019 -0400

    Merge pull request #19158 from d-m-u/successive_automation_task_runs_should_not_remove_params
    
    Successive automation task runs shouldn't change the original params
    
    (cherry picked from commit c86ed36af6ffa758dfaf20e213b44b065ac8fd3f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1713072

simaishi pushed a commit that referenced this pull request Sep 5, 2019
@bdunne @simaishi
Merge pull request #19158 from d-m-u/successive_automation_task_runs_…
e7df03e 
…should_not_remove_params

Successive automation task runs shouldn't change the original params

(cherry picked from commit c86ed36)

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

simaishi commented Sep 5, 2019

Hammer backport details:

$ git log -1
commit e7df03e6cd3a414ba97de4a41fbcad772f7af3d2
Author: Brandon Dunne <[email protected]>
Date:   Thu Aug 22 08:16:53 2019 -0400

    Merge pull request #19158 from d-m-u/successive_automation_task_runs_should_not_remove_params
    
    Successive automation task runs shouldn't change the original params
    
    (cherry picked from commit c86ed36af6ffa758dfaf20e213b44b065ac8fd3f)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1745197

@d-m-u d-m-u deleted the successive_automation_task_runs_should_not_remove_params branch September 26, 2019 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmcculloug gmcculloug gmcculloug approved these changes

@bdunne bdunne bdunne approved these changes

@syncrou syncrou Awaiting requested review from syncrou

Assignees

@bdunne bdunne

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

Successfully merging this pull request may close these issues.
6 participants
@d-m-u @gmcculloug @bdunne @miq-bot @simaishi @tinaafitz