Please set the example to /etc/pagure/pagure_plugins.cfg. This prevents it from being byte-compiled, among other things.

PAGURE_PLUGIN_CONFIG, please?

@xyzzyx Aside from my two comments, this looks pretty good. :100:

PAGURE_PLUGIN_CONFIG

I can change it, no problem. I wonder though, wouldn't it be better to keep the same name of the environment variable since they are they same thing? The only difference is that the variable is either set in the config file or in the environment. Having 2 names for the same thing could be confusing. A different option is to change the environment variable to PAGURE_PLUGIN_CONFIG but this change will break any current instance that is using it.

@xyzzyx We can change it and deprecate the old name, slate that for removal in 6.0. Until that point, using either should trigger the same behavior.

@xyzzyx Can you rename the file in files to have the .cfg.sample extension like how the pagure.cfg.sample file exists?

Since nothing is defined here, I don't think it's really worth adding this here. Having it in the doc/configuration and in files where you've nicely documented it is sufficient, imho

Oh nice, thanks for starting this!

I'm not sure this last line is worth keeping in the doc, it's an internal implementation details (runserver is only there for development purposes, you do not want to use this for a production deployment, it's not even shipped in the releases tarball)

We should check if PAGURE_PLUGIN is set and emit a DeprecationWarning if it is and keep the plugin_config.from_envvar which is below otherwise we're breaking backward compatibility.

That's what I meant above, this warning should be in the pagure code itself, runserver is really just a convenience tool but it's not something people will/should actually use

plugin_config.from_envvar has not been removed.

@pingou Should I also remove all other mentions of runserver.py in the documentation?

@xyzzyx Please do so. :smile:

:thumbsup:

After some research, there is a built-in mechanism in python for deprecation warnings as part of the warning module: https://docs.python.org/3/library/warnings.html I believe this is the proper way to do this/.

I have one final comment but I've tested this locally and it works fine so once we've tweaked our warning I think we can get this in :)

I was aware of the warnings module but it's not used anywhere else in Pagure. All the other warnings in Pagure use the logging module via logging.getLogger and just print to the log file. Do you prefer to use the warnings module here? Or maybe both?

I'd say we should merge as-is and change to using warnings module everywhere in a followup PR.

The other warnings are warnings that are important to log but I think this deserves even being even more prominent since it may impact pagure instance running.

I'm ok with both (I like the belt+suspender approach for these kinds of changes)

:thumbsup:

Looks good to me, I'll give it a final test locally and merge it (but tomorrow at this point)

I'm not sure why the jenkins test has failed.. If I run tests/test_style.py locally it passes. I'll try to commit again.

@ngompa @pingou Can you please check tests/test_style.py? It passes for me locally but jenkins says it failed. I have no clue why.

Flake8 seems to be the issue
You can see the output here: https://ci.centos.org/job/pagure-pr/3443/artifact/pagure/results_fedora-pip-py3/py-test_style/view/

Copied here as after a while they disapear from jenkins:

-------------------- >> begin captured stdout << ---------------------
(b"/pagure/pagure/api/__init__.py:36:1: E402 module level import not at top of file\n/pagure/pagure/hooks/files/default_hook.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/default_hook.py'\n/pagure/pagure/hooks/files/git_multimail.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/git_multimail.py'\n/pagure/pagure/hooks/files/mirror.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/mirror.py'\n/pagure/pagure/hooks/files/pagure_block_unsigned.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/pagure_block_unsigned.py'\n/pagure/pagure/hooks/files/pagure_force_commit_hook.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/pagure_force_commit_hook.py'\n/pagure/pagure/hooks/files/pagure_hook.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/pagure_hook.py'\n/pagure/pagure/hooks/files/pagure_hook_requests.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/pagure_hook_requests.py'\n/pagure/pagure/hooks/files/pagure_hook_tickets.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/pagure_hook_tickets.py'\n/pagure/pagure/hooks/files/rtd_hook.py:0:1: E902 FileNotFoundError: [Errno 2] No such file or directory: '/pagure/pagure/hooks/files/rtd_hook.py'\n/pagure/pagure/lib/git_auth.py:707:13: E741 ambiguous variable name 'l'\n/pagure/pagure/ui/filters.py:350:21: F504 '...' % ... has unused named argument(s): templ_edited\n/pagure/pagure/ui/repo.py:850:40: E741 ambiguous variable name 'l'\n", None)
--------------------- >> end captured stdout << ----------------------

Is there anything that I can or should do to solve this? None of the errors seem to be related to any file touched by this patch.

Nice addition! :thumbsup:

And jenkins is back to green, let's merge this quickly before it changes its mind! :)