cli: fix SOPEL_CONFIG env var broken usage

Author: Exirel

sopel/cli/run.py

@@ -356,22 +356,24 @@ def get_configuration(options):
     return settings
 
 
-def get_pid_filename(options, pid_dir):
-    """Get the pid file name in ``pid_dir`` from the given ``options``.
+def get_pid_filename(settings, pid_dir):
+    """Get the pid file name in ``pid_dir`` from the given ``settings``.
 
-    :param options: command line options
+    :param settings: Sopel config
+    :type settings: :class:`sopel.config.Config`
     :param str pid_dir: path to the pid directory
     :return: absolute filename of the pid file
 
-    By default, it's ``sopel.pid``, but if a configuration filename is given
-    in the ``options``, its basename is used to generate the filename, as:
+    By default, it's ``sopel.pid``, but if the configuration's basename is not
+    ``default`` then it will be used to generate the pid file name as
     ``sopel-{basename}.pid`` instead.
     """
     name = 'sopel.pid'
-    if options.config and options.config != 'default':
-        basename = os.path.basename(options.config)
-        if basename.endswith('.cfg'):
-            basename = basename[:-4]
+    if settings.basename != 'default':
+        filename = os.path.basename(settings.filename)
+        basename, ext = os.path.splitext(filename)
+        if ext != '.cfg':
+            basename = filename
         name = 'sopel-%s.pid' % basename
 
     return os.path.abspath(os.path.join(pid_dir, name))
@@ -402,18 +404,18 @@ def command_start(opts):
     """Start a Sopel instance"""
     # Step One: Get the configuration file and prepare to run
     try:
-        config_module = get_configuration(opts)
+        settings = get_configuration(opts)
     except config.ConfigurationError as e:
         tools.stderr(e)
         return ERR_CODE_NO_RESTART
 
-    if config_module.core.not_configured:
+    if settings.core.not_configured:
         tools.stderr('Bot is not configured, can\'t start')
         return ERR_CODE_NO_RESTART
 
     # Step Two: Handle process-lifecycle options and manage the PID file
-    pid_dir = config_module.core.pid_dir
-    pid_file_path = get_pid_filename(opts, pid_dir)
+    pid_dir = settings.core.pid_dir
+    pid_file_path = get_pid_filename(settings, pid_dir)
     pid = get_running_pid(pid_file_path)
 
     if pid is not None and tools.check_pid(pid):
@@ -432,7 +434,7 @@ def command_start(opts):
         pid_file.write(str(os.getpid()))
 
     # Step Three: Run Sopel
-    ret = run(config_module, pid_file_path)
+    ret = run(settings, pid_file_path)
 
     # Step Four: Shutdown Clean-Up
     os.unlink(pid_file_path)
@@ -471,7 +473,7 @@ def command_stop(opts):
     logger.setup_logging(settings)
 
     # Get Sopel's PID
-    filename = get_pid_filename(opts, settings.core.pid_dir)
+    filename = get_pid_filename(settings, settings.core.pid_dir)
     pid = get_running_pid(filename)
 
     if pid is None or not tools.check_pid(pid):
@@ -510,7 +512,7 @@ def command_restart(opts):
     logger.setup_logging(settings)
 
     # Get Sopel's PID
-    filename = get_pid_filename(opts, settings.core.pid_dir)
+    filename = get_pid_filename(settings, settings.core.pid_dir)
     pid = get_running_pid(filename)
 
     if pid is None or not tools.check_pid(pid):
@@ -585,18 +587,18 @@ def command_legacy(opts):
 
     # Step Two: Get the configuration file and prepare to run
     try:
-        config_module = get_configuration(opts)
+        settings = get_configuration(opts)
     except config.ConfigurationError as e:
         tools.stderr(e)
         return ERR_CODE_NO_RESTART
 
-    if config_module.core.not_configured:
+    if settings.core.not_configured:
         tools.stderr('Bot is not configured, can\'t start')
         return ERR_CODE_NO_RESTART
 
     # Step Three: Handle process-lifecycle options and manage the PID file
-    pid_dir = config_module.core.pid_dir
-    pid_file_path = get_pid_filename(opts, pid_dir)
+    pid_dir = settings.core.pid_dir
+    pid_file_path = get_pid_filename(settings, pid_dir)
     old_pid = get_running_pid(pid_file_path)
 
     if old_pid is not None and tools.check_pid(old_pid):
@@ -649,7 +651,7 @@ def command_legacy(opts):
         pid_file.write(str(os.getpid()))
 
     # Step Four: Initialize and run Sopel
-    ret = run(config_module, pid_file_path)
+    ret = run(settings, pid_file_path)
     os.unlink(pid_file_path)
     if ret == -1:
         os.execv(sys.executable, ['python'] + sys.argv)

sopel/cli/utils.py

@@ -266,6 +266,9 @@ def add_common_arguments(parser):
     ``config`` and ``configdir`` options that can be used to find and load
     Sopel's settings.
 
+    The default value for ``config`` is either the value of the environment
+    variable ``SOPEL_CONFIG``, or the string ``default``.
+
     .. seealso::
 
         The :func:`sopel.cli.utils.load_settings` function uses an ``options``
@@ -274,15 +277,17 @@ def add_common_arguments(parser):
     """
     parser.add_argument(
         '-c', '--config',
-        default='default',
+        default=os.environ.get('SOPEL_CONFIG') or 'default',
         metavar='filename',
         dest='config',
         help=inspect.cleandoc("""
             Use a specific configuration file.
             A config name can be given and the configuration file will be
-            found in Sopel\'s homedir (defaults to ``~/.sopel/default.cfg``).
+            found in Sopel's homedir (defaults to ``~/.sopel/default.cfg``).
             An absolute pathname can be provided instead to use an
             arbitrary location.
+            When the ``SOPEL_CONFIG`` environment variable is set and not
+            empty, it is used as the default value.
         """))
     parser.add_argument(
         '--config-dir',
@@ -302,14 +307,14 @@ def load_settings(options):
     :raise sopel.config.ConfigurationError: raised when configuration is
                                             invalid
 
-    This function loads Sopel's settings from one of these sources:
+    This function loads Sopel's settings from ``options.config``. This option's
+    value should be from one of these sources:
 
-    * value of ``options.config``, if given,
-    * ``SOPEL_CONFIG`` environment variable, if no option is given,
-    * otherwise the ``default`` configuration is loaded,
+    * given by the command line argument,
+    * ``SOPEL_CONFIG`` environment variable, if the argument is not used,
+    * otherwise it should default to ``default``,
 
-    then loads the settings and returns it as a :class:`~sopel.config.Config`
-    object.
+    then it returns it as a :class:`~sopel.config.Config` object.
 
     If the configuration file can not be found, a
     :exc:`sopel.config.ConfigurationNotFound` error will be raised.
@@ -320,17 +325,11 @@ def load_settings(options):
         ``config`` and ``configdir``.
 
         The :func:`sopel.cli.utils.add_common_arguments` function should be
-        used to add these options to the argument parser.
+        used to add these options to the argument parser. This function is also
+        responsible for using the environment variable or the default value.
 
     """
-    # Default if no options.config or no env var or if they are empty
-    name = 'default'
-    if options.config:
-        name = options.config
-    elif 'SOPEL_CONFIG' in os.environ:
-        name = os.environ['SOPEL_CONFIG'] or name  # use default if empty
-
-    filename = find_config(options.configdir, name)
+    filename = find_config(options.configdir, options.config)
 
     if not os.path.isfile(filename):
         raise config.ConfigurationNotFound(filename=filename)

test/cli/test_cli_run.py

@@ -17,6 +17,14 @@
 )
 
 
+TMP_CONFIG = """
+[core]
+owner = testnick
+nick = TestBot
+enable = coretasks
+"""
+
+
 @contextmanager
 def cd(newdir):
     prevdir = os.getcwd()
@@ -366,41 +374,30 @@ def test_get_configuration(tmpdir):
         assert result.core.owner == 'TestName'
 
 
-def test_get_pid_filename_default():
+def test_get_pid_filename_default(configfactory):
     """Assert function returns the default filename from given ``pid_dir``"""
     pid_dir = '/pid'
-    parser = build_parser()
-    options = parser.parse_args(['legacy'])
+    settings = configfactory('default.cfg', TMP_CONFIG)
 
-    result = get_pid_filename(options, pid_dir)
+    result = get_pid_filename(settings, pid_dir)
     assert result == pid_dir + '/sopel.pid'
 
 
-def test_get_pid_filename_named():
+def test_get_pid_filename_named(configfactory):
     """Assert function returns a specific filename when config (with extension) is set"""
     pid_dir = '/pid'
-    parser = build_parser()
+    settings = configfactory('test.cfg', TMP_CONFIG)
 
-    # With extension
-    options = parser.parse_args(['legacy', '-c', 'test.cfg'])
-
-    result = get_pid_filename(options, pid_dir)
+    result = get_pid_filename(settings, pid_dir)
     assert result == pid_dir + '/sopel-test.pid'
 
-    # Without extension
-    options = parser.parse_args(['legacy', '-c', 'test'])
-
-    result = get_pid_filename(options, pid_dir)
-    assert result == pid_dir + '/sopel-test.pid'
 
-
-def test_get_pid_filename_ext_not_cfg():
+def test_get_pid_filename_ext_not_cfg(configfactory):
     """Assert function keeps the config file extension when it is not cfg"""
     pid_dir = '/pid'
-    parser = build_parser()
-    options = parser.parse_args(['legacy', '-c', 'test.ini'])
+    settings = configfactory('test.ini', TMP_CONFIG)
 
-    result = get_pid_filename(options, pid_dir)
+    result = get_pid_filename(settings, pid_dir)
     assert result == pid_dir + '/sopel-test.ini.pid'
 
 

test/cli/test_cli_utils.py

@@ -15,11 +15,20 @@
     find_config,
     get_many_text,
     green,
+    load_settings,
     red,
     yellow,
 )
 
 
+TMP_CONFIG = """
+[core]
+owner = testnick
+nick = TestBot
+enable = coretasks
+"""
+
+
 @contextmanager
 def cd(newdir):
     prevdir = os.getcwd()
@@ -206,3 +215,84 @@ def test_get_many_text(items, expected):
         'elements {first} and {second}',
         'elements {left}, and {last}')
     assert result == expected
+
+
+def test_load_settings(config_dir):
+    config_dir.join('config.cfg').write(TMP_CONFIG)
+    parser = argparse.ArgumentParser()
+    add_common_arguments(parser)
+
+    options = parser.parse_args([
+        '--config-dir', config_dir.strpath,
+        '-c', 'config.cfg',
+    ])
+
+    settings = load_settings(options)
+    assert isinstance(settings, config.Config)
+    assert settings.basename == 'config'
+
+
+def test_load_settings_arg_priority_over_env(monkeypatch, config_dir):
+    monkeypatch.setenv('SOPEL_CONFIG', 'fromenv')
+
+    config_dir.join('fromenv.cfg').write(TMP_CONFIG)
+    config_dir.join('fromarg.cfg').write(TMP_CONFIG)
+    parser = argparse.ArgumentParser()
+    add_common_arguments(parser)
+
+    options = parser.parse_args([
+        '--config-dir', config_dir.strpath,
+        '-c', 'fromarg',
+    ])
+
+    settings = load_settings(options)
+    assert isinstance(settings, config.Config)
+    assert settings.basename == 'fromarg'
+
+
+def test_load_settings_default(config_dir):
+    config_dir.join('default.cfg').write(TMP_CONFIG)
+    parser = argparse.ArgumentParser()
+    add_common_arguments(parser)
+
+    options = parser.parse_args(['--config-dir', config_dir.strpath])
+
+    settings = load_settings(options)
+    assert isinstance(settings, config.Config)
+
+
+def test_load_settings_default_env_var(monkeypatch, config_dir):
+    monkeypatch.setenv('SOPEL_CONFIG', 'config')
+
+    config_dir.join('config.cfg').write(TMP_CONFIG)
+    parser = argparse.ArgumentParser()
+    add_common_arguments(parser)
+
+    options = parser.parse_args(['--config-dir', config_dir.strpath])
+
+    settings = load_settings(options)
+    assert isinstance(settings, config.Config)
+    assert settings.basename == 'config'
+
+
+def test_load_settings_default_not_found(config_dir):
+    parser = argparse.ArgumentParser()
+    add_common_arguments(parser)
+
+    options = parser.parse_args(['--config-dir', config_dir.strpath])
+
+    with pytest.raises(config.ConfigurationNotFound):
+        load_settings(options)
+
+
+def test_load_settings_invalid(config_dir):
+    parser = argparse.ArgumentParser()
+    add_common_arguments(parser)
+
+    options = parser.parse_args([
+        '--config-dir', config_dir.strpath,
+        '-c', 'config.cfg',
+    ])
+
+    with pytest.raises(ValueError):  # no [core] section
+        load_settings(options)