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 #12288 - disable transactions for plan phase in run host job action #74

Merged
merged 1 commit into from Dec 8, 2015

Conversation

iNecas
Copy link
Member

@iNecas iNecas commented Nov 12, 2015

Otherwise we lose the link from the task to the job invocation. Since
in this case, the execution plan is not driving a transaction between
local database and external systems, we can turn this logic off for
this action.

With this patch (that depends on Dynflow/dynflow#174),
when there is an issue with running the job on proxy (as in case when the proxy
is just down), we show the tasks on per-host part as failed, with posibility to
show details, as with any other error.

@ares
Copy link
Member

ares commented Nov 19, 2015

I suppose tests failed because it needs newer dynflow?

@iNecas
Copy link
Member Author

iNecas commented Nov 19, 2015

Yes

…tion

Otherwise we lose the link from the task to the job invocation. Since
in this case, the execution plan is not driving a transaction between
local database and external systems, we can turn this logic off for
this action.

Due to dependencies on Dynflow, we need to add the dependency to dynflow 0.8.8
although foreman-tasks works also with older versions.
@iNecas iNecas force-pushed the dont-use-transactions-for-ssh branch from ae63f74 to 5f73947 Compare November 26, 2015 14:44
@iNecas
Copy link
Member Author

iNecas commented Nov 27, 2015

The tests are green now

@@ -10,6 +11,9 @@ def resource_locks

def plan(job_invocation, host, template_invocation, proxy, connection_options = {})
action_subject(host, :job_name => job_invocation.job_name)
link!(job_invocation)
link!(template_invocation)

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for moving those three lines is to create the links before any error can be raised?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to do that as soon as possible, before any other error occurs, so that one can get to the errors form the linked resource

@adamruzicka
Copy link
Contributor

Works as expected, ACK

iNecas added a commit that referenced this pull request Dec 8, 2015
Fixes #12288 - disable transactions for plan phase in run host job action
@iNecas iNecas merged commit 4aaaafd into theforeman:master Dec 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants