nbgitpuller
nbgitpuller copied to clipboard
Deprecate/Remove `app` query parameter and the NBGITPULLER_APP default value
We have a app query parameter that is accepted in the /git-pull endpoint that influences the redirection path to unless urlpath is also set. A default value for this query parameter can be set via NBGITPULLER_APP.
I'm considering if we should remove it or attempt to deprecate it etc to reduce the complexity.
Background
@manics wrote
[...] there's an undocumented environment variable
NBGITPULLER_APPhttps://github.com/jupyterhub/nbgitpuller/blob/fdd54bdd2b9c6452dcbeff8acf07a5accf7b1b64/nbgitpuller/handlers.py#L149 Do you know it's significance?
I replied
@manics it seem to have been added by me four years ago in https://github.com/jupyterhub/nbgitpuller/pull/41 :open_mouth:
I did some git history inspection:
- #41 added NBGITPULLER_APP, a default value for an
appparameter acceptingnotebookandlab.- Documentation about it updated in #48
- Documentation on constructing the url removed in favor of using a link generator in #62
Overall, it seems that it does the single thing of "if set to lab", the web handler for
/git-pullendpoint will default to prepending/lab/treeto a post git-pull redirection path. It won't affectgitpullerthe CLI though.I conclude that the https://jupyterhub.github.io/nbgitpuller/link only crafts links using
urlpathdirectly though, and theNBGITPULLER_APPis a default value for aappquery parameter, which only has an influence ifurlpathisn't set.I'd love to see this env var and the entire
appquery parameter removed to reduce complexity, but breaking existing links isn't fun. @yuvipanda do you have a suggestion with regards to something to do with the logic about having anappparameter and/or the NBGITPULLER_APP env which is theappparameters default value?
Thank you for researching this thoroughly and opening this up, @consideRatio!
I think when external URLs that can live anywhere (emails, course websites, etc) are counted, we should try really hard to not break them. So my suggestion is to let these be, and perhaps leave a note saying 'do not add more things here, thanks'.
Thanks for the quick followup @yuvipanda! Sounds reasonable to me, what do you think @manics?
Deprecating (but not removing) the app parameter makes sense.
What's the plan for the NBGITPULLER_APP env-var- remove it completely?
Let's deprecate it (maybe a warning message if it is detected?) but leave it in?