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

Fixes #20692 - Add API for rerunning jobs #288

Merged
merged 8 commits into from
Mar 2, 2018
Merged

Fixes #20692 - Add API for rerunning jobs #288

merged 8 commits into from
Mar 2, 2018

Conversation

adamruzicka
Copy link
Contributor

@adamruzicka adamruzicka commented Oct 19, 2017 •
edited

  • tests

@iNecas
Copy link
Member

iNecas commented Oct 31, 2017

Setting Waiting on contributor until the tests are added. Haven't tested yet, but looks good code-wise, except missing permissions definition (which is the reason why the tests are failing)

@iNecas
Copy link
Member

iNecas commented Nov 30, 2017

👮 is not happy

houndci-bot
houndci-bot reviewed Feb 26, 2018
View reviewed changes
test/functional/api/v2/job_invocations_controller_test.rb Outdated
.expects(:validate_job_category)
.with(@invocation.job_category)
.returns(@invocation.job_category)
post :rerun, :id => @invocation.id
Copy link

Choose a reason for hiding this comment

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

Use keyword arguments instead of positional arguments for http call: post.

houndci-bot
houndci-bot reviewed Feb 26, 2018
View reviewed changes
test/functional/api/v2/job_invocations_controller_test.rb Outdated
.expects(:validate_job_category)
.with(@invocation.job_category)
.returns(@invocation.job_category)
post :rerun, :id => @invocation.id
Copy link

Choose a reason for hiding this comment

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

Use keyword arguments instead of positional arguments for http call: post.

@adamruzicka
Copy link
Contributor Author

[test]

@iNecas
Copy link
Member

iNecas commented Mar 1, 2018

We got conflicts, please rebase and run tests manually, as we have broken master at the moment

iNecas
iNecas suggested changes Mar 1, 2018
View reviewed changes
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.

When I was testing with hammer, it succeeded when rerunning all hosts, but failed, when using --failed-only true:

hammer-cli master → bundle exec bin/hammer job-invocation rerun --id 674
Job invocation was rerun as 677
hammer-cli master → bundle exec bin/hammer job-invocation rerun --id 674 --failed-only true
ERF42-4343 [Foreman::Exception]: Cannot resolve hosts without a user

@iNecas
Copy link
Member

iNecas commented Mar 1, 2018

Backtrace:

| Foreman::Exception: ERF42-4343 [Foreman::Exception]: Cannot resolve hosts without a user
 | /home/inecas/Projects/ws/foreman-rex/foreman_remote_execution/app/models/targeting.rb:35:in `resolve_hosts!'
 | /home/inecas/Projects/ws/foreman-rex/foreman_remote_execution/app/lib/actions/remote_execution/run_hosts_job.rb:22:in `plan'
 | /home/inecas/Projects/ws/foreman-rex/dynflow/lib/dynflow/action.rb:473:in `block (3 levels) in execute_plan'

@iNecas
Copy link
Member

iNecas commented Mar 1, 2018

Adding link to hammer for better orientation theforeman/hammer_cli_foreman_remote_execution#18

houndci-bot
houndci-bot reviewed Mar 1, 2018
View reviewed changes
app/models/job_invocation_composer.rb Outdated
if @host_ids
{ :search_query => Targeting.build_query_from_hosts(@host_ids), :targeting_type => job_invocation.targeting.targeting_type }
search_query = @host_ids.empty? ? 'name ^ ()' : Targeting.build_query_from_hosts(@host_ids)
Copy link

Choose a reason for hiding this comment

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

Unnecessary spacing detected.

adamruzicka
adamruzicka commented Mar 1, 2018
View reviewed changes
app/models/job_invocation_composer.rb Outdated
@@ -194,10 +194,12 @@ def concurrency_control_params
end

def targeting_params
base = { :user_id => job_invocation.targeting.user_id }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it would make sense to use User.current here instead of copying over the original user

Copy link
Member

Choose a reason for hiding this comment

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

Definitely should be the current user

@iNecas
Copy link
Member

iNecas commented Mar 2, 2018

Running tests manually http://ci.theforeman.org/job/test_plugin_pull_request/4760/

@iNecas
Copy link
Member

iNecas commented Mar 2, 2018

Tested and works like a charm. The relevant tests in CI work as well. Thanks @adamruzicka

@iNecas iNecas merged commit cbca376 into theforeman:master Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iNecas iNecas iNecas requested changes

@houndci-bot houndci-bot houndci-bot left review comments

Assignees
No one assigned
Labels
Needs re-review Needs testing
Projects
None yet
Milestone
No milestone
4 participants
@adamruzicka @iNecas @houndci-bot @theforeman-bot