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 #13274 - the smart proxy service can now load #19

Merged
merged 1 commit into from Jan 20, 2016

Conversation

ohadlevy
Copy link
Member

No description provided.

@stbenjam
Copy link
Member

Do you know why this fixes it? I can't reproduce locally on any ruby version I try. I can see why we'd need smart_proxy_dynflow, but why the version?

@ohadlevy
Copy link
Member Author

@stbenjam what loads the version? since its not required anywhere, there is no auto loader enabled.

@stbenjam
Copy link
Member

It was moved to the after_activation hook. https://github.com/theforeman/smart_proxy_remote_execution_ssh/blob/master/lib/smart_proxy_remote_execution_ssh/plugin.rb#L15

Is it needed before? I think we should probably only require it in one place

@ohadlevy
Copy link
Member Author

ohadlevy commented Jan 19, 2016 via email

@stbenjam
Copy link
Member

Just looking at other examples (like this one), plugin requires version itself.

Then smart_proxy_dynflow require could go where you put it, and we remove the 2 from the after_activation hook?

I don't really have any strong preference, just want to make sure it's in a logical place and fixes the ordering issues.

@ohadlevy
Copy link
Member Author

ohadlevy commented Jan 19, 2016 via email

@iNecas
Copy link
Member

iNecas commented Jan 20, 2016

I haven't found better place than in this PR. Reproduced, tested, worked, merged

iNecas added a commit that referenced this pull request Jan 20, 2016
fixes #13274 - the smart proxy service can now load
@iNecas iNecas merged commit 34df656 into theforeman:master Jan 20, 2016
@stbenjam
Copy link
Member

Cool, thanks @iNecas !

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