Merge ~marcusboden/juju-lint:bug/1939438 into juju-lint:master

[Marcus Boden]

use values in config file

Values set in $HOME/.config/juju-lint/config.yaml were ignored, due to
the default values of the ArgumentParser. This patch
- removes the default values from the parser
- adds default values to the config_default.yaml
- adds tests to check if the config files are read correctly

Closes-Bug: #1939438

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:d775903a348ad41ac014fc9273f3546781029d8e
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/34/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1342/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1671/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/34//rebuild

review: Needs Fixing (continuous-integration)

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:d775903a348ad41ac014fc9273f3546781029d8e
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/35/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1343/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1672/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/35//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:d775903a348ad41ac014fc9273f3546781029d8e
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/36/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1346/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1675/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/36//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:d775903a348ad41ac014fc9273f3546781029d8e
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/37/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1347/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1676/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/37//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:e1874deeef6d1daa1f3d6d50843dc1d3e15dba6c
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/38/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1350/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1679/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/38//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:e1874deeef6d1daa1f3d6d50843dc1d3e15dba6c
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/39/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1351/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1680/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/39//rebuild

review: Approve (continuous-integration)

[Gabriel Cocenza (gabrielcocenza)]

Hi Marcus, Thanks for contributing with the project.

This patch generally LGTM, but I left some comments and suggestions.

I would like to ask you to double check if unit tests [0] are passing because when I cloned your repo, the "test_default_config" is failing because the "config" object is a OrderedDict. Not sure if it's a different version of python, confuse etc.

[0] https://pastebin.canonical.com/p/fFNggCGz9M/

review: Needs Information

[Marcus Boden (marcusboden)]

> Hi Marcus, Thanks for contributing with the project.
>
> This patch generally LGTM, but I left some comments and suggestions.
>
> I would like to ask you to double check if unit tests [0] are passing because
> when I cloned your repo, the "test_default_config" is failing because the
> "config" object is a OrderedDict. Not sure if it's a different version of
> python, confuse etc.
>
> [0] https://pastebin.canonical.com/p/fFNggCGz9M/

I think it's not the OrderedDict that causes the problem, but the test reading values from ~/.config/juju-lint/config.yaml.
T've changed the test so that it doesn't read that config file anymore.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:493cc40c1d1ee7fe07845406981063a9cd8e10e4
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/45/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1502/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1750/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/45//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:493cc40c1d1ee7fe07845406981063a9cd8e10e4
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/47/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1505/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1753/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/47//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:493cc40c1d1ee7fe07845406981063a9cd8e10e4
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/47/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1505/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1754/
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1753/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/47//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:493cc40c1d1ee7fe07845406981063a9cd8e10e4
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/48/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1506/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1755/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/48//rebuild

review: Needs Fixing (continuous-integration)

[Erhan Sunar (esunar)]

inline

review: Needs Fixing

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:fffc38c23ed3aa4d416597d6e133640c576edeb5
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/51/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1509/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1758/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/51//rebuild

review: Needs Fixing (continuous-integration)

[Marcus Boden (marcusboden)]

~gabrielcocenza, here's the output of make test on my machine, as discussed on mattermost:

https://pastebin.canonical.com/p/Ghq4rjD8P2/

[Gabriel Cocenza (gabrielcocenza)]

Thanks Marcus.

Just two little things that I found in the code and we will be ready to go.

review: Needs Fixing

[Gabriel Cocenza (gabrielcocenza)]

LGTM

review: Approve

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:c57bb5506938892ca27da0fe0c943d30919abed8
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/52/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1510/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1759/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/52//rebuild

review: Approve (continuous-integration)

[Erhan Sunar (esunar)]

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 1a1351f416a008b6dbef2c42872f6881aedd1905

diff --git a/jujulint/config.py b/jujulint/config.py
index 45c5618..be2453e 100644
--- a/jujulint/config.py
+++ b/jujulint/config.py
@@ -37,7 +37,6 @@ class Config(Configuration):
             "-l",
             "--log-level",
             type=str,
-            default="info",
             nargs="?",
             help="The default log level, valid options are info, warn, error or debug",
             dest="logging.loglevel",
@@ -46,7 +45,6 @@ class Config(Configuration):
             "-d",
             "--output-dir",
             type=str,
-            default="",
             metavar="DIR",
             help=(
                 "Dump gathered cloud state data into %(metavar)s. "
@@ -67,7 +65,6 @@ class Config(Configuration):
         self.parser.add_argument(
             "-c",
             "--config",
-            default="lint-rules.yaml",
             help="File to read lint rules from. Defaults to `lint-rules.yaml`",
             dest="rules.file",
         )
@@ -99,7 +96,6 @@ class Config(Configuration):
         self.parser.add_argument(
             "--logfile",
             "-L",
-            default=None,
             help="File to log to in addition to stdout",
             dest="logging.file",
         )
@@ -107,7 +103,6 @@ class Config(Configuration):
             "--format",
             "-F",
             choices=["text", "json"],
-            default="text",
             help="Format for output",
         )
 
diff --git a/jujulint/config_default.yaml b/jujulint/config_default.yaml
index 662df79..e9a3bcb 100644
--- a/jujulint/config_default.yaml
+++ b/jujulint/config_default.yaml
@@ -21,8 +21,13 @@
 #   sudo: 'juju-user'
 
 logging:
-  level: INFO
+  loglevel: INFO
   file: jujulint.log
 
 rules:
   file: lint-rules.yaml
+
+output:
+  folder:
+
+format: text
diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py
new file mode 100644
index 0000000..1d9060d
--- /dev/null
+++ b/tests/unit/test_config.py
@@ -0,0 +1,116 @@
+#!/usr/bin/python3
+"""Test correct reading of the all config files."""
+
+import os
+import sys
+
+import yaml
+
+from jujulint.config import Config
+
+config_file = f"{Config().config_dir()}/config.yaml"
+builtin_open = open
+builtin_isfile = os.path.isfile
+
+mock_file_config = """
+logging:
+  loglevel: WARN
+  file: test.log
+
+rules:
+  file: file-test.yaml
+
+
+output:
+  folder: folder-test
+  dump: dump-test
+
+format: json
+"""
+
+
+def patch_user_config(mocker):
+    """Patch calls to ~/.config/juju-lint/config.yaml.
+
+    The confuse package reads the config file in .config, if it exists. We have
+    to mock the os.isfile call and create a mock open function that only
+    intercepts calls to that file.
+    """
+
+    def my_mock_open(*args, **kwargs):
+        if args[0] == config_file:
+            return mocker.mock_open(read_data=mock_file_config)(*args, **kwargs)
+        return builtin_open(*args, **kwargs)
+
+    def side_effect(filename):
+        if filename == config_file:
+            return True
+        else:
+            return builtin_isfile(filename)
+
+    mocker.patch("os.path.isfile", side_effect=side_effect)
+    mocker.patch("builtins.open", my_mock_open)
+
+
+def test_config_file(mocker):
+    """Tests if the config entries set in the .config/juju-lint/config.yaml are correctly applied.
+
+    The values in .config/juju-lint/config.yaml should overwrite the fileds in config_default.yaml
+    """
+    expected_config = yaml.safe_load(mock_file_config)
+    patch_user_config(mocker)
+    config = Config()
+    # you cannot do config.get(), so we iterate over the toplevel keys
+    for key in expected_config.keys():
+        assert config[key].get() == expected_config[key]
+
+
+def test_default_config(mocker):
+    """Tests if the default values are correctly read from config_default.yaml."""
+    default_config = {
+        "logging": {"loglevel": "INFO", "file": "jujulint.log"},
+        "rules": {"file": "lint-rules.yaml"},
+        "output": {"folder": None},
+        "format": "text",
+    }
+
+    # Don't use the .config/juju-lint/config.yaml!
+    def side_effect(filename):
+        if filename == config_file:
+            return False
+        else:
+            return builtin_isfile(filename)
+
+    mocker.patch("os.path.isfile", side_effect=side_effect)
+
+    config = Config()
+    for key in default_config.keys():
+        assert config[key].get() == default_config[key]
+
+
+def test_parser_options(mocker):
+    """Tests if cli options overwrite the options in config files."""
+    cli_config = {
+        "logging": {"loglevel": "DEBUG", "file": "cli.log"},
+        "rules": {"file": "cli-rules.yaml"},
+        "output": {"folder": "cli/folder"},
+        "format": "json",
+    }
+    test_args = sys.argv + [
+        "-F",
+        cli_config["format"],
+        "-l",
+        cli_config["logging"]["loglevel"],
+        "-d",
+        cli_config["output"]["folder"],
+        "-c",
+        cli_config["rules"]["file"],
+        "-L",
+        cli_config["logging"]["file"],
+    ]
+
+    mocker.patch.object(sys, "argv", test_args)
+    patch_user_config(mocker)
+    config = Config()
+    for key in cli_config.keys():
+        assert config[key].get() == cli_config[key]