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

improve timeout handling #197

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Conversation

mattp-
Copy link
Contributor

@mattp- mattp- commented Jul 30, 2019

the timeout kwarg previously was only being used for polling timeout. we
can also pass it as a low kwarg and use it for urllib connection timeout
as well.

mattp- added 2 commits July 30, 2019 14:33
the timeout kwarg previously was only being used for polling timeout. we
can also pass it as a low kwarg and use it for urllib connection timeout
as well.
mattp- added 2 commits July 30, 2019 20:40
since these timeout kwargs should coincide with upstream response, we
give a bit of buffer to let salt-api time out gracefully before forcing
it first.
@eplodn
Copy link

eplodn commented Aug 1, 2019

Will this solve #198 ?

@mattp-
Copy link
Contributor Author

mattp- commented Aug 2, 2019

@eplodn no, I think what you're actually looking for is https://github.com/saltstack/pepper/blob/develop/pepper/cli.py#L647
its gated off because its not compatible with tornado salt-api, but if you run cherrypy it will work as is. the pr to fix it has been sitting for a few months in salts queue

@eplodn
Copy link

eplodn commented Aug 5, 2019

I'm using pepper 0.7.5 and salt 2018.3.3 with cherrypy and it doesn't work, what am I missing?

@mattp-
Copy link
Contributor Author

mattp- commented Aug 6, 2019

@eplodn timeouts in salt are tricky, there is no 'timeout' in the sense you are thinking for a long running job that will abort. if a minion is responding to the master polling for status, it will run forever. This PR should hypothetically do something on the client side for what you are looking for but it it is not ready for merge atm

mattp- added 4 commits October 4, 2019 17:22
--json was broken with the introduction of this because client isnt set
/ shortcircuited when json is used. We can add the equivalent logic in parse_cmd
instead.
mixing low data with kwargs has been deprecated forever, it should be
safe to change this now.
@nicholasmhughes
Copy link
Collaborator

Howdy! 👋

Apologies for the long delay on this integrating this code. We certainly appreciate your effort!

Pepper is getting some renewed focus and love, and we're checking in to see if you'd like to update this against the latest version of the base branch and ensure that all merge conditions are met.

If the items addressed by this contribution have already been fixed/added, are no longer an issue, or you no longer have a desire to work on this... please let us know and/or close out this PR.

If we don't hear from you within 7 days, we'll assume the code has been abandoned and close it out. If you'd like to revisit this in the future, you can always open a new PR.

Thank you, and we hope to see you active in the Salt ecosystem in the future!

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

Successfully merging this pull request may close these issues.

3 participants