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 #17239 - Increase scap content validation timeout #227

Merged
merged 1 commit into from Dec 14, 2016

Conversation

shlomizadok
Copy link
Member

No description provided.

private

def timeout
Setting[:proxy_request_timeout] > 120 ? Setting[:proxy_request_timeout] : 120
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need something like Setting[:proxy_request_timeout] && Setting[:proxy_request_timeout] > 120 for tests to pass.

private

def timeout
(Setting[:proxy_request_timeout] && Setting[:proxy_request_timeout] > 120) ? Setting[:proxy_request_timeout] : 120

Choose a reason for hiding this comment

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

Omit parentheses for ternary conditions.

@shlomizadok
Copy link
Member Author

test failures are unrelated

@xprazak2
Copy link
Contributor

Seems to take care of timeout issue for me, but my proxy gets killed by kernel 2 out of 3 times because I seem to be out of memory. This may be because I am running a devel proxy in a box, but I wonder if we should handle this somehow. The exceptions are RestClient::ServerBrokeConnection when killed during validation, Errno::ECONNREFUSED when fetch_policies_for_scap_content is called. Your thoughts?

@shlomizadok
Copy link
Member Author

Weird... it didn't happen to me with the file from the issue. I also run devel proxy (on my machine - not in a box...)

@xprazak2
Copy link
Contributor

xprazak2 commented Dec 13, 2016

I just re-tested in different env and it worked flawlessly. The issues I had above seem to be tied to my setup. Jenkins failures unrelated, seems like ready to merge if there are no additional comments.

@xprazak2 xprazak2 merged commit a726ad0 into theforeman:master Dec 14, 2016
@xprazak2
Copy link
Contributor

Merged, thanks @shlomizadok!

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