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

close the requests.Session() object used by dcos_api_session #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

margaret
Copy link
Contributor

@margaret margaret commented May 10, 2018

High-level description

What features does this change enable? What bugs does this change fix?

The requests.Session() object is not used as a context manager so it needs to have close() called explicitly.

Corresponding DC/OS tickets (obligatory)

These JIRA ticket(s) must be updated (ideally closed) in the moment this PR lands:

Related tickets (optional)

Other tickets related to this change:

  • QUALITY- Foo the Bar so it stops Bazzing.

Related dcos-launch and dcos PRs

Is this change going to be propagated up into another repo? Test the change by bumping the dcos-test-utils SHA to point to these changes to test it. Link the corresponding PRs here:

dcos/dcos#2860

Checklist for all PRs

  • Included a test which will fail if code is reverted but test is not. If there is no test please explain here:
  • Include a test in dcos-integration-tests in https://github.com/dcos/dcos or explain why this is not applicable:
  • Include a test in https://github.com/dcos/dcos-launch or explain why this is not applicable:

Integration tests were run and

  • AWS Cloudformation Simple passed (link to job: )
  • Onprem-AWS passed (link to job: )
  • Onprem-GCE passed (link to job: )

PLEASE FILL IN THE TEMPLATE ABOVE / DO NOT REMOVE ANY SECTIONS ABOVE THIS LINE

Instructions and review process

What is the review process and when will my changes land?

All PRs require 2 approvals using GitHub's pull request reviews.

Reviewers should be:

  • Developers who understand the code being modified.
  • Developers responsible for code that interacts with or depends on the code being modified.

It is best to proactively ask for 2 reviews by @mentioning the candidate reviewers in the PR comments area. The responsibility is on the developer submitting the PR to follow-up with reviewers and make sure a PR is reviewed in a timely manner.

@margaret
Copy link
Contributor Author

enterprise integration test is https://jira.mesosphere.com/browse/QUALITY-2127

Copy link
Contributor

@jongiddy jongiddy left a comment

Choose a reason for hiding this comment

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

Not sure if this change will go ahead, given dcos/dcos#2860 (comment)

However, if it does, you should move the api.wait_for_dcos() into the try block. This ensures that the session is also closed if an exception occurs during the long-running wait_for_dcos(). The session is created in the create() method, so this is safe.

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.

2 participants