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 #26352 - ActiveJob task arguments loading #396

Merged
merged 1 commit into from Mar 15, 2019

Conversation

ezr-ondrej
Copy link
Member

ActiveJob arguments loading is broken by: Dynflow/dynflow#317

@@ -118,7 +118,8 @@ def main_action
else
execution_plan_action.input
end
@main_action = active_job_action(args['job_class'], args['job_arguments'])
args = args['job_data'] || args.slice('job_class')
Copy link
Member

Choose a reason for hiding this comment

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

For the tasks created before this change, shouldn't this be:

args = args['job_data'].try { |data| data['arguments'] } || args['job_arguments']
@main_action = active_job_action(args['job_class'], args['arguments'])

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you are right 👍 thanks.

Copy link
Member

Choose a reason for hiding this comment

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

One small ask: please add a comment that we need the fallback for job_arguments for fobs from Dynflow <=1.2.1, for the future version of us that would wonder why we're doing this here :)

Copy link
Member

@iNecas iNecas 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, good otherwise

@ezr-ondrej
Copy link
Member Author

I believe now we are backwards compatible :)

@iNecas
Copy link
Member

iNecas commented Mar 14, 2019

There is one more occurrence of job_arguments in

return execution_plan_action.input['job_arguments'] if active_job?
, would be good to fix that one as well while touching it.

@ezr-ondrej
Copy link
Member Author

I have pulled the logic into its method, just left one comment, where I am unsure.

@iNecas iNecas merged commit c505485 into theforeman:master Mar 15, 2019
@iNecas
Copy link
Member

iNecas commented Mar 15, 2019

Thanks @ezr-ondrej

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