Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Deprecate/Remove app query parameter and the NBGITPULLER_APP default value #247

Open
consideRatio opened this issue Jan 18, 2022 · 4 comments

Comments

@consideRatio
Copy link
Member

consideRatio commented Jan 18, 2022

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_APP

app_env = os.getenv('NBGITPULLER_APP', default='notebook')

Do you know it's significance?

I replied

@manics it seem to have been added by me four years ago in #41 😮

I did some git history inspection:

Overall, it seems that it does the single thing of "if set to lab", the web handler for /git-pull endpoint will default to prepending /lab/tree to a post git-pull redirection path. It won't affect gitpuller the CLI though.

I conclude that the https://jupyterhub.github.io/nbgitpuller/link only crafts links using urlpath directly though, and the NBGITPULLER_APP is a default value for a app query parameter, which only has an influence if urlpath isn't set.

I'd love to see this env var and the entire app query 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 an app parameter and/or the NBGITPULLER_APP env which is the app parameters default value?

@yuvipanda
Copy link
Contributor

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'.

@consideRatio
Copy link
Member Author

Thanks for the quick followup @yuvipanda! Sounds reasonable to me, what do you think @manics?

@manics
Copy link
Member

manics commented Jan 19, 2022

Deprecating (but not removing) the app parameter makes sense.
What's the plan for the NBGITPULLER_APP env-var- remove it completely?

@yuvipanda
Copy link
Contributor

Let's deprecate it (maybe a warning message if it is detected?) but leave it in?

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

3 participants