is_appengine=False in testbed (like it used to) #1760
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1760 +/- ##
======================================
Coverage 100% 100%
======================================
Files 22 22
Lines 1991 1991
======================================
Hits 1991 1991
Continue to review full report at Codecov.
|
Whether testbed tests "are appengine" is debatable, but historically this function has returned False in testbed tests. This behavior was inadvertently (and unnecessarily) changed in PR urllib3#1704. This commit undoes that regression for testbed tests.
|
This issue turned up when I tried to pull the latest version of urllib3 into google and run it against all our tests. |
|
The AppVeyor error is an issue I need to solve, and the coverage is in fact still 100%, see https://codecov.io/gh/urllib3/urllib3/pull/1760/tree. So our CI should be happy even though it's not! Does this version now pass all your tests? In any case I think we'll need a review from @theacodes again as I don't know anything about App Engine |
|
@zevdg We're in need of a new AppEngine champion. No current maintainers know much of AppEngine and we'd love to keep supporting the sub-module but I'd prefer to not pull Thea into the conversation every time it comes up as she's got her own work and schedule. Surely there's someone at Google that can help us maintain this piece of code? |
|
I've reached out to the people who are in charge of the python runtimes in the App Engine team. They agreed that someone on their team should be in touch with you. I think one of them will be taking a look at this change and joining the convo. |
|
I'm the team lead at Google for the App Engine Python 2.7 runtime, and this change looks good to me. Feel free to loop me in to help review PRs affecting appengine.py and _appengine_environ.py. This change reverts most of PR #1704, aside from the removal of the checks for the now-decommissioned Managed VMs environment. Checking for APPENGINE_RUNTIME then checking the SERVER_SOFTWARE prefix is the correct way to determine if you're running in the python27 runtime. Newer runtimes (python37 and later) define GAE_ENV and GAE_RUNTIME instead, and don't have the default restriction on sockets, so it's correct for them not to be detected as App Engine here. |
|
That's great to hear, thanks for the followup @myelin! Ok, now let's try to fix the CI... |
|
Thanks @myelin for lending a hand. Are you willing to be tagged on all PRs related to App Engine then or is there a better way to contact you? |
|
Yep! Go ahead and tag me on App Engine PRs; I'm happy to help out there in future. |
Whether testbed tests "are appengine" is debatable, but historically this function has returned False in testbed tests. This behavior was inadvertently (and unnecessarily) changed in PR urllib3#1704. This commit undoes that regression for testbed tests.
Whether testbed tests "are appengine" is debatable, but historically
this function has returned False in testbed tests. This behavior was
inadvertently (and unnecessarily) changed in PR #1704. This commit
undoes that regression for testbed tests.