Fix get_current_version_name() so it doesn't raise KeyErrors in non-GAE environments

[Codonaut]

Right now get_current_version_name() will raise a KeyError if called outside of an appengine app. Here's a portion of a stacktrace showing this:

File "/home/jenkins/workspace/optimizely-test-e2e/src/www/zip_imports/gaek-0.3.0.zip/gaek/environ.py", line 96, in is_default_version

    version = version or get_current_version_name()

  File "/home/jenkins/.google-cloud-sdk/platform/google_appengine/google/appengine/api/modules/modules.py", line 91, in get_current_version_name

    result = os.environ['CURRENT_VERSION_ID'].split('.')[0]

  File "/home/jenkins/workspace/optimizely-test-e2e/.virtualenv/www/test/lib/python2.7/UserDict.py", line 40, in __getitem__

    raise KeyError(key)

KeyError: 'CURRENT_VERSION_ID'

This usually isn't a huge deal because generally gaek is used in a running app. This is however a problem when running remote API scripts that interact with application code. In those cases the 'CURRENT_VERSION_ID' environment variable is not populated, and a KeyError is raised.

I think suppressing that error is the natural thing to do here, especially since it's not at all clear that an error would be raised when calling gaek.environ.is_default_version

[erichiggins]

@Codonaut Thanks for the PR! Are there other methods, either from modules or some other package that could also throw an exception? I seem to recall others which rely on a stubbed API being available, which often isn't outside of the GAE environment (or a stubbed local environment).

If there are more, is there a way to cover them all at a higher level?

[Codonaut]

Good question-- I just took a look and get_current_module_name also looks susceptible to a KeyError in the case of a missing environment variable. From what I can tell the other modules functions that are used as-is in gaek all make API calls to get their results, and I think that should work even in a remote API context.

I'll fix get_current_module_name in the same way I did get_current_version_name

[erichiggins]

There are a few functions from google.appengine.api.modules that rely on RPC calls. Notably, those which are used to get information from modules other than the "current" one.

Examples:

• get_modules
• get_versions
• get_default_version
• get_hostname

Going from memory, these will raise an exception due to the modules_service_pb not being available. I forget the exact details or exception, but it should be easy to reproduce.

[Codonaut]

I think this PR is useful regardless of whether or not it fixes all of these functions. The two KeyErrors that get raised are addressed by this diff, and fixing the other class of errors could go in another PR imo.

[erichiggins]

I guess my point is that this PR alters the intent (convenience of fewer imports) by wrapping some methods and changing their behavior. That might break things for people and it adds the responsibility of consistency -- make sure they all work offline/unstubbed. The docs would also need to be updated to make those changes clear to users. It's a lot more work, which is why I was wondering if you had ideas on how to handle all cases instead of just these few. Does that make sense?

Alternatively, we could add a convenience method to stub them all out (e.g. gaek.environment.stub_all())

[erichiggins]

I thought of another approach that may be a bit easier to implement, without the backwards compatible drawback.

Each pass-through method could be wrapped and renamed with _safe or _offline. These wrapped functions should behave the same, unless the API isn't stubbed, in which case they can return None or another reasonable alternative.

Example:

gaek.get_modules()
>>> KeyError

gaek.get_modules_safe()
>>> None

Thoughts?

[Codonaut]

Hey Eric sorry for the slow reply, been busy with some other work.

The _safe approach seems good to me. I'll try to pursue that and update this PR.

[Codonaut]

@erichiggins what do you think of adding an optional safe boolean kwarg? That way the environ module namespace won't have a bunch of nearly identical functions.

E.g.

gaek.get_modules()
>>> KeyError

gaek.get_modules(safe=True)
>>> None

[Codonaut]

Actually, I just went through the module calling all the functions in a remote API context and in a plain ol' python shell context and I don't think that we should stub all the functions-- imo the only two functions we should look at are get_current_version_name and get_current_module_name. The reason is that every other function works in a remote API context, it's only these two that raise errors. I think it's reasonable for gaek to assume that it's either being used in (1) a running GAE app or (2) in a GAE remote API context. In a regular old shell blowing up with API proxy errors seems like the right behavior to me.

[erichiggins]

Ok. So do you propose just adding an optional safe parameter to those two functions, or would it be more explicit to make gaek-specific wrappers (e.g. get_current_module_safe())? For the reasons I mentioned before, I think I'd prefer the wrapped function, but adding a custom parameter would be fine too. If you prefer the parameter, I think something longer & more specific might be better (offline_safe=True perhaps).

Either way, can you please take a moment to update the docs to reflect the changes in this PR as well?

[Codonaut]

Using wrapper functions is fine with me. And yup, I'll update the docs accordingly in my next update here.

[erichiggins]

@Codonaut Yo, just wanted to check in on this. No rush, but don't want to block you if it's ready to go.

[Codonaut]

Hey @erichiggins, sorry I didn't see the notification for your last message. Sorry to leave you hanging for a while, I've been getting pulled in a few different directions so it took me a little bit to come back to this.

Anyway, I think this PR is in a pretty good state now. I added a get_current_module_name_safe and a get_current_version_name_safe, and then also added _safe versions of the other functions that consume those two. Let me know what you think!

[Codonaut]

Ok, I fixed the typo and got myself out of the git hell that suddenly popped up. Thanks for the fast review Eric!

[erichiggins]

Awesome! Thanks for the PR, @Codonaut !