search

jupyter-server / enterprise_gateway

A lightweight, multi-tenant, scalable and secure gateway that enables Jupyter Notebooks to share resources across distributed clusters such as Apache Spark, Kubernetes and others.
https://jupyter-enterprise-gateway.readthedocs.io/en/latest/
Other
620 stars 223 forks source link

Replace "is defined" in Jinja templates if conditions #1267

Closed starskyreverie closed 1 year ago

starskyreverie commented 1 year ago

Removed "is defined" for Jinja templates if conditions. I believe if variable is preferred to if variable is defined because if variable will return false if the variable is defined but empty, e.g. like an empty string or array. It's also cleaner unless the intended behavior is actually to return true even with an empty string/array passed as the env variable.

For example, using if variable:

image

as compared to if variable is defined...:

image
kevin-bates commented 1 year ago

Hi @pq43 - was it your intention to close this PR?

starskyreverie commented 1 year ago

Woops, my bad, reopened! It is failing some tests though, which I'm kinda confused about - but I will take a look and see if I can fix them :)

starskyreverie commented 1 year ago 
manifest for elyra/demo-base:3.3.0.dev0 not found: manifest unknown: manifest unknown

This is that it looks like the error is: 729... any idea what's going on? I'm not so sure if it's related to this PR or not, it looks like it's happening on main too

kevin-bates commented 1 year ago

Hi @pq43 - don't worry about the build failure - I see what is happening and it's unrelated to your PR. I'll submit a fix for it soon.

I have a question regarding your changes and the "falsey" behavior.

So, is that possible if a user wanted to set the CPUs to 0 (or any numeric value to 0)? It seems like with this change, the updates to the numeric-based entries could never be set to 0 resulting in their "default values" being used (since they won't appear to be defined to 0).

starskyreverie commented 1 year ago

Oh yeah, that's definitely true - I actually had that same concern as well. I think removing is defined only makes sense for non-numeric values, thank you for spotting that.

I just made the change now to add the is defined behavior back for numeric values, and only removed the is defined for non numeric values.

Though, now I can also see the argument that this code might feel inconsistent and unnecessary, so no worries if you think this PR is better left unmerged :)

starskyreverie commented 1 year ago

Should be good now, sorry about all that, and no worries if this isn't something you think is best

kevin-bates commented 1 year ago

Hi @pq43 - PR #1268 fixes the build issue. Would you mind updating to main so we can ensure the builds are green? Thank you.