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

Refs #16058 - extract common code for remote execution and ansible #198

Merged
merged 1 commit into from
Sep 7, 2016
Merged

Refs #16058 - extract common code for remote execution and ansible #198

merged 1 commit into from
Sep 7, 2016

Conversation

iNecas
Copy link
Member

@iNecas iNecas commented Sep 2, 2016 •
edited
Loading

@iNecas iNecas mentioned this pull request Sep 2, 2016
Merged
4 tasks
adamruzicka
adamruzicka reviewed Sep 2, 2016
View reviewed changes
app/lib/actions/helpers/with_delegated_action.rb
case proxy
when :not_defined
if klass.is_a?(String)
raise _(_'No proxy defined for execution')
Copy link
Contributor

Choose a reason for hiding this comment

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

One _ should be enough

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@adamruzicka
Copy link
Contributor

Gemspec for foreman-tasks-core is missing

@iNecas
Copy link
Member Author

iNecas commented Sep 2, 2016

@adamruzicka added the misisng spec

adamruzicka
adamruzicka reviewed Sep 7, 2016
View reviewed changes
lib/foreman_tasks_core/callback_action.rb
end

def run
SmartProxyDynflowCore::Callback::Request.send_to_foreman_tasks(input[:callback], input[:data])
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering if this is the right place for this to be in? I know this will probably be only used from within SmartProxyDynflowCore, but ForemanTasks{,Core} doesn't have any kind of dependency on SPDC.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, we still reference it in SharableAction, but there is not other easy way how to do that. Anyway, no reason for this class to be here

@adamruzicka
Copy link
Contributor

I inlined one last comment, but I don't feel strongly about it. Either way let's wait for Mr. Jenkins' approval and get this merged

tl; dr: APJ

@adamruzicka
Copy link
Contributor

No more comments from me

@iNecas
Copy link
Member Author

iNecas commented Sep 7, 2016

I squashed the commits and waiting for jenkins to merge and release.

@iNecas
Copy link
Member Author

iNecas commented Sep 7, 2016

Jenskins is green, I'm going to release new gems so that foreman_ansible and forman_remote_execution can proceed

@iNecas iNecas merged commit 181b39d into theforeman:master Sep 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs testing Not yet reviewed
Projects
None yet
Milestone
No milestone
3 participants
@iNecas @adamruzicka @theforeman-bot