Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Adding script name to 'global_config' during application setup #3735

Merged
merged 8 commits into from
Jan 28, 2024

Conversation

adroullier
Copy link
Contributor

This code extends pyramid startup scripts in python/bin directory (pserve, prequest, pshell, pviews, proutes, ptweens) with additional info about the calling script. The additional info is passed to the **main(global_config, settings) function as part of global_config dict during wsgi setup as 'script'.
global_config can contain custom values loaded from the ini file, so to avoid unwanted overwriting of a existing 'script' value I've added a extra condition for comapatibility.

In some cases it makes sense to call different code whether the application is created as server (bin/pserve) or for a single request (bin/prequest). For example setting up internal caching modules, maintenance or other background functions.

@adroullier adroullier marked this pull request as draft October 24, 2023 12:22
@adroullier adroullier marked this pull request as ready for review October 24, 2023 13:01
Copy link
Member

@mmerickel mmerickel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. could you please tweak these to use config_vars.setdefault('__script__', 'pshell') as a simpler one-liner?
  2. Is there a way to make this work with pyramid.paster.bootstrap as well, or only the scripts? I believe this use case is not addressed here and was the topic of the recent mailing list posts triggering this work. I'm thinking __script__ == 'bootstrap' in that case.

@adroullier
Copy link
Contributor Author

1: no problem.

2: pyramid.paster.bootstrap is called in the scripts proutes, pshell, ptweens, pviews. In this case the script name is passed to bootstrap() as part of the options parameter (which is then passed to the callback get_app()). If bootstrap() gets called from other parts of the code there is no such info.
You are right, it makes sense to add info to boostrap(), too. Maybe '__script__'='bootstrap' could be used for all these scripts (proutes, pshell, ptweens, pviews). It is more consistent.

@mmerickel
Copy link
Member

mmerickel commented Oct 26, 2023

It could be the default value going back to your logic that won’t set it if already defined. Then individual scripts can still override it as you’re doing?

@adroullier
Copy link
Contributor Author

The scripts (except pserve, prequest) call pyramid.paster.bootstrap, so the script name is set before bootstrap() is called and shouldn't be a problem.

I had a look at the email discussion and pyramid code again. The original question was if there is way to have more infos in the context of the ApplicationCreated event. The event is triggered in pyramid.config.Configurator.make_wsgi_app(). The PR does not change this behaviour and I don't think there is a way to change the event.

@mmerickel
Copy link
Member

The user would need to forward the setting from global _config into their app but at least it’s an automatic setting versus one they have to set/maintain in their config files.

@mmerickel mmerickel self-requested a review October 30, 2023 20:58
Copy link
Member

@mmerickel mmerickel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the status of this from your perspective @adroullier? Trying to understand if you think this is done or if there's still a plan to handle the bootstrap case. I still think we should define __script__ if bootstrap is used directly, not just via these p* scripts.

Rest of the PR looks great and I'd be happy to merge it.

@adroullier
Copy link
Contributor Author

Sorry I haven't mentioned it in my last comment.
Actually I haven't found a use case or point where custom pyramid code is called after calling pyramid.paster.bootstrap() (and I think that is what you are referring to?).
If bootstrap()is called by custom code the developer knows the environment anyway before calling bootstrap(). So adding the script name there means just adding a few lines of unnecessary code.
Or is there a mistake in my argumentation?

I can also add the code to set the name to 'bootstrap', like

pyramid.paster.bootstrap() line 127

def bootstrap(config_uri, request=None, options=None):
    # ...
    if options is None:
        options = dict()
    options.setdefault('__script__', 'bootstrap')
    app = get_app(config_uri, options=options)
    env = prepare(request)
    env['app'] = app
    return env

@mmerickel
Copy link
Member

mmerickel commented Dec 1, 2023

Well the goal is to still add that so the app can differentiate the scenario from a normal startup using that __script__ protocl without relying on every user setting it themselves prior to calling bootstrap. I think it's worth adding that in the spirit of this PR.

@mmerickel mmerickel merged commit 55f9eb0 into Pylons:main Jan 28, 2024
32 checks passed
@mmerickel
Copy link
Member

Fixes #3734.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants