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.

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

Fixes #32047 - Match proxy actions using task_id #617

Merged
merged 1 commit into from
Mar 22, 2021

Conversation

adamruzicka
Copy link
Contributor

When we're retrieving outputs of proxy actions we get back a serialized
execution plan, walk its actions and look for an action with either expected
action_class or operation name. This is somewhat flaky.

With this change we only check one field in the actions' input and that is
task_id. First action found with matching value is considered the source of
output for the current task.

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

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

One comment, otherwise makes sense. I wonder why we didn't do that like this before? Is there any moment or use case, where we wouldn't have the task id available in the parsed response?

action['class'] == proxy_action_name || action.fetch('input', {})['proxy_operation_name'] == proxy_operation_name
end
proxy_data.fetch('output', {})
response['actions'].detect { |action| action['input']['task_id'] == task.id }
Copy link
Member

Choose a reason for hiding this comment

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

given how action.fetch('input', {}) was used defensively before, are we sure action['input'] is always a hash? or should we also write that more defensively?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there's an action there should be an input and if there's an input it is a hash. Otoh playing it safe is almost free so I can do it that way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

When we're retrieving outputs of proxy actions we get back a serialized
execution plan, walk its actions and look for an action with either expected
action_class or operation name. This is somewhat flaky.

With this change we only check one field in the actions' input and that is
task_id. First action found with matching value is considered the source of
output for the current task.
@adamruzicka
Copy link
Contributor Author

FTR, I would consider this temporary. It will most likely change when we start retrieving outputs in batches

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

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

Thanks @adamruzicka, merging!

@ares ares merged commit cd69cd6 into theforeman:master Mar 22, 2021
@adamruzicka adamruzicka deleted the proxy-action-matching branch March 22, 2021 14:54
@ares
Copy link
Member

ares commented Apr 28, 2021

This probably caused the regression that for Ansible, I see Error loading data from proxy: NoMethodError - undefined method fetch' for nil:NilClass` with every update of the console (1 sec interval) until it finishes. I'll see if reverting helps when I have a moment

@adamruzicka
Copy link
Contributor Author

adamruzicka commented Apr 28, 2021

To be honest I'd be surprised if this was the cause as this doesn't really change how we access the data.

Edit: What I wrote before still holds, but the return type is different.

@adamruzicka
Copy link
Contributor Author

#629

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants