Merge ~gabrielcocenza/juju-lint:bug/1978739 into juju-lint:master

[Gabriel Cocenza]

applied pattern of bootstack charms for make and tox commands.

- added pre-commit support.
- solved lint issues.
- removed debug flag to build snap. If the build fails, it should
  stop the CI and not open the shell from the snap.
- added an aggregator-kubernetes to solve pre-commit yaml check.
- changed tox and make commands to match scripts on the bootstack CI.

Closes-Bug: #1978739
Partial-Bug: #1978841

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

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

[Eric Chen (eric-chen)]

review: Approve

[Martin Kalcok (martin-kalcok)]

Thanks for the cleanup effort. It's definitely needed. However I have two comments:

* We were talking with the rest of the bootstack that we won't be forcing pyptoject.toml into new charms/projects just yet. We already have tox and Makefiles and adding pyptoject on top seems unnecessary.

* pre-commits should use tox to execute actions to ensure that the same versions of tools (e.g. black, flake8) will be used as in the CI pipeline. Example of such pre-commit (by Robert Gildein): https://pastebin.canonical.com/p/gBjX3qqfkY/

review: Needs Fixing

[Gabriel Cocenza (gabrielcocenza)]

Thanks for your review Martin. I've removed the pyptoject.toml and changed the pre-commit as you said, to use the same environment of tox.

[Martin Kalcok (martin-kalcok)]

One small question in-line, otherwise LGTM.

[Martin Kalcok (martin-kalcok)]

review: Approve

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

Change successfully merged at revision ea3f3020a6e0d77cdb4ffc7f29d7f7ff3b9b112d

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
 new file mode 100644
index 0000000..c327de3
--- /dev/null
+++ b/.pre-commit-config.yaml
@@ -0,0 +1,19 @@
+repos:
+  - repo: https://github.com/pre-commit/pre-commit-hooks
+    rev: v4.0.1
+    hooks:
+      - id: check-executables-have-shebangs
+      - id: check-merge-conflict
+      - id: end-of-file-fixer
+      - id: trailing-whitespace
+      - id: check-added-large-files
+      - id: check-json
+      - id: check-yaml
+        args: ["--unsafe"]
+  - repo: local
+    hooks:
+      - id: lint
+        name: lint
+        entry: tox -e lint
+        language: system
+        types: [ python ]
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 7e46600..1e5c9b9 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -22,11 +22,17 @@ git clone git+ssh://<LAUNCHPAD_USER>@git.launchpad.net/juju-lint
 cd juju-lint/
 ```
 
-You can use the environments created by `tox` for development:
+Create and activate a virtualenv with the development requirements:
 
 ```shell
-tox --notest -e unit
-source .tox/unit/bin/activate
+make dev-environment
+```
+
+This step is mandatory, otherwise a message like this will appear if you try to commit:
+
+```shell
+$ git commit
+`pre-commit` not found.  Did you forget to activate your virtualenv?
 ```
 
 After making your changes you can run the CI without the need of building and installing the snap by using:
@@ -40,9 +46,12 @@ python3 -m jujulint.cli <PATH_TO_YAML> -c <PATH_TO_RULE_FILE> -l debug
 
 ```shell
 make lint            # check code style
-make test            # unit tests
+make reformat        # reformat the code using black and isort
+make unittests       # run unit tests
+make functional      # run functional tests
+make test            # run lint, unittests and functional
 make build           # build the snap
-make clean           # clean the snapcraft lxd containers
+make clean           # clean the snapcraft lxd containers and the snap files created
 ```
 
 ## Canonical Contributor Agreement
diff --git a/Makefile b/Makefile
index 452acd4..49303b3 100644
--- a/Makefile
+++ b/Makefile
@@ -1,15 +1,56 @@
+help:
+	@echo "This project supports the following targets"
+	@echo ""
+	@echo " make help - show this text"
+	@echo " make clean - remove unneeded files"
+	@echo " make dev-environment - setup the development environment"
+	@echo " make build - build the snap"
+	@echo " make lint - run flake8, black --check and isort --check-only"
+	@echo " make reformat - run black and isort and reformat files"
+	@echo " make unittests - run the tests defined in the unittest subdirectory"
+	@echo " make functional - run the tests defined in the functional subdirectory"
+	@echo " make test - run lint, proof, unittests and functional targets"
+	@echo " make pre-commit - run pre-commit checks on all the files"
+	@echo ""
+
+
 lint:
-	tox -e lintverbose
+	@echo "Running lint checks"
+	@tox -e lint
+
+unittests:
+	@echo "Running unit tests"
+	@tox -e unit
 
-test:
-	tox -e unit
+test: lint unittests functional
+	@echo "Tests completed for the snap."
 
-format-code:
-	tox -e format-code
+reformat:
+	@echo "Reformat files with black and isort"
+	tox -e reformat
 
 build:
-	snapcraft --use-lxd --debug
+	@echo "Building the snap"
+	@snapcraft --use-lxd
 
 clean:
-	snapcraft clean --use-lxd
+	@echo "Cleaning snap"
+	@snapcraft clean --use-lxd
+	@echo "Cleaning existing snap builds"
+	@find . -name "*.snap" -delete
+
+dev-environment:
+	@echo "Creating virtualenv and installing pre-commit"
+	@virtualenv -p python3 .venv
+	@.venv/bin/pip install -r tests/requirements.txt
+	@.venv/bin/pre-commit install
+
+functional: build
+	@echo "Executing functional tests using built snap"
+	@tox -e func
+
+pre-commit:
+	@tox -e pre-commit
 
+# The targets below don't depend on a file
+.PHONY: help clean dev-environment build lint reformat unittests functional test pre-commit
diff --git a/contrib/fcb-stein-bionic.yaml b/contrib/fcb-stein-bionic.yaml
index 863058f..6d9f16f 100644
--- a/contrib/fcb-stein-bionic.yaml
+++ b/contrib/fcb-stein-bionic.yaml
@@ -6,4 +6,4 @@
 !include includes/openstack.yaml
 !include includes/operations.yaml
 !include includes/saas.yaml
-!include includes/aggregator-openstack.yaml
 \ No newline at end of file
+!include includes/aggregator-openstack.yaml
diff --git a/contrib/fcb-ussuri-bionic-ovs.yaml b/contrib/fcb-ussuri-bionic-ovs.yaml
index 51de731..86d624d 100644
--- a/contrib/fcb-ussuri-bionic-ovs.yaml
+++ b/contrib/fcb-ussuri-bionic-ovs.yaml
@@ -6,4 +6,4 @@
 !include includes/openstack.yaml
 !include includes/operations.yaml
 !include includes/saas.yaml
-!include includes/aggregator-openstack.yaml
 \ No newline at end of file
+!include includes/aggregator-openstack.yaml
diff --git a/contrib/fcb-ussuri-bionic.yaml b/contrib/fcb-ussuri-bionic.yaml
index 6fade6b..76e899b 100644
--- a/contrib/fcb-ussuri-bionic.yaml
+++ b/contrib/fcb-ussuri-bionic.yaml
@@ -6,4 +6,4 @@
 !include includes/openstack.yaml
 !include includes/operations.yaml
 !include includes/saas.yaml
-!include includes/aggregator-openstack.yaml
 \ No newline at end of file
+!include includes/aggregator-openstack.yaml
diff --git a/contrib/fcb-ussuri-focal-ovs.yaml b/contrib/fcb-ussuri-focal-ovs.yaml
index a29ed0e..0e989a0 100644
--- a/contrib/fcb-ussuri-focal-ovs.yaml
+++ b/contrib/fcb-ussuri-focal-ovs.yaml
@@ -6,4 +6,4 @@
 !include includes/openstack.yaml
 !include includes/operations.yaml
 !include includes/saas.yaml
-!include includes/aggregator-openstack.yaml
 \ No newline at end of file
+!include includes/aggregator-openstack.yaml
diff --git a/contrib/fcb-ussuri-focal.yaml b/contrib/fcb-ussuri-focal.yaml
index 2e6f2eb..8b53cfe 100644
--- a/contrib/fcb-ussuri-focal.yaml
+++ b/contrib/fcb-ussuri-focal.yaml
@@ -6,4 +6,4 @@
 !include includes/openstack.yaml
 !include includes/operations.yaml
 !include includes/saas.yaml
-!include includes/aggregator-openstack.yaml
 \ No newline at end of file
+!include includes/aggregator-openstack.yaml
diff --git a/contrib/fcb-yoga-focal.yaml b/contrib/fcb-yoga-focal.yaml
index 4293a7d..632a73a 100644
--- a/contrib/fcb-yoga-focal.yaml
+++ b/contrib/fcb-yoga-focal.yaml
@@ -6,4 +6,4 @@
 !include includes/openstack.yaml
 !include includes/operations.yaml
 !include includes/saas.yaml
-!include includes/aggregator-openstack.yaml
 \ No newline at end of file
+!include includes/aggregator-openstack.yaml
diff --git a/contrib/includes/aggregator-kubernetes.yaml b/contrib/includes/aggregator-kubernetes.yaml
 new file mode 100644
index 0000000..95c2099
--- /dev/null
+++ b/contrib/includes/aggregator-kubernetes.yaml
@@ -0,0 +1,16 @@
+operations charms: &operations-charms
+  - *operations-kubernetes-mandatory-charms
+  - *operations-mandatory-charms
+  - *operations-mandatory-deps
+  - *operations-mandatory-subs
+  - *operations-optional-charms
+  - *operations-optional-subs
+
+kubernetes charms: &kubernetes-charms
+  - *kubernetes-mandatory-charms
+  - *kubernetes-optional-charms
+
+known charms:
+  - ubuntu
+  - *operations-charms
+  - *kubernetes-charms
diff --git a/contrib/includes/release/stein.yaml b/contrib/includes/release/stein.yaml
index 7319287..3195046 100644
--- a/contrib/includes/release/stein.yaml
+++ b/contrib/includes/release/stein.yaml
@@ -1,4 +1,4 @@
 # add prometheus ceph exporter if openstack version is lesser than ussuri
 
 operations openstack mandatory release: &operations-openstack-mandatory-release
-  - prometheus-ceph-exporter
 \ No newline at end of file
+  - prometheus-ceph-exporter
diff --git a/contrib/includes/release/ussuri.yaml b/contrib/includes/release/ussuri.yaml
index ddf9beb..4080368 100644
--- a/contrib/includes/release/ussuri.yaml
+++ b/contrib/includes/release/ussuri.yaml
@@ -2,4 +2,4 @@
 # Instead, use ceph-dashboard
 
 operations openstack mandatory release: &operations-openstack-mandatory-release
-  - ceph-dashboard
 \ No newline at end of file
+  - ceph-dashboard
diff --git a/contrib/includes/release/yoga.yaml b/contrib/includes/release/yoga.yaml
index ddf9beb..4080368 100644
--- a/contrib/includes/release/yoga.yaml
+++ b/contrib/includes/release/yoga.yaml
@@ -2,4 +2,4 @@
 # Instead, use ceph-dashboard
 
 operations openstack mandatory release: &operations-openstack-mandatory-release
-  - ceph-dashboard
 \ No newline at end of file
+  - ceph-dashboard
diff --git a/contrib/includes/series/focal.yaml b/contrib/includes/series/focal.yaml
index 4084113..4771d14 100644
--- a/contrib/includes/series/focal.yaml
+++ b/contrib/includes/series/focal.yaml
@@ -1,3 +1,3 @@
 openstack loadbalancer: &openstack-loadbalancer
   - openstack-loadbalancer
-operations openstack mandatory series: &operations-openstack-mandatory-series []
 \ No newline at end of file
+operations openstack mandatory series: &operations-openstack-mandatory-series []
diff --git a/contrib/kubernetes.yaml b/contrib/kubernetes.yaml
index 27eeba6..4e90b94 100644
--- a/contrib/kubernetes.yaml
+++ b/contrib/kubernetes.yaml
@@ -4,20 +4,4 @@
 !include includes/operations.yaml
 !include includes/kubernetes.yaml
 !include includes/saas.yaml
-
-operations charms: &operations-charms
- - *operations-kubernetes-mandatory-charms
- - *operations-mandatory-charms
- - *operations-mandatory-deps
- - *operations-mandatory-subs
- - *operations-optional-charms
- - *operations-optional-subs
-
-kubernetes charms: &kubernetes-charms
- - *kubernetes-mandatory-charms
- - *kubernetes-optional-charms
-
-known charms:
- - ubuntu
- - *operations-charms
- - *kubernetes-charms
+!include includes/aggregator-kubernetes.yaml
diff --git a/setup.py b/setup.py
index b6b3dc9..9314d60 100644
--- a/setup.py
+++ b/setup.py
@@ -16,9 +16,9 @@
 # You should have received a copy of the GNU General Public License
 # along with this program. If not, see <http://www.gnu.org/licenses/>.
 """Setuptools packaging metadata for juju-lint."""
+import warnings
 
 import setuptools
-import warnings
 
 warnings.simplefilter("ignore", UserWarning)  # Older pips complain about newer options.
 
diff --git a/tests/requirements.txt b/tests/requirements.txt
index 2575eb3..d31d3fb 100644
--- a/tests/requirements.txt
+++ b/tests/requirements.txt
@@ -1,5 +1,6 @@
 # Module requirements
 black
+colorama
 flake8
 flake8-colors
 flake8-docstrings
@@ -13,3 +14,4 @@ pytest
 pytest-cov
 pytest-html
 pytest-mock
+pre-commit
diff --git a/tests/test_jujulint.py b/tests/test_jujulint.py
index e99401a..6e40b74 100644
--- a/tests/test_jujulint.py
+++ b/tests/test_jujulint.py
@@ -787,6 +787,7 @@ applications:
     }
 
     def test_check_spaces_detect_mismatches(self, linter, mocker):
+        """Test that check spaces mismatch gives warning message."""
         mock_log: mock.MagicMock = mocker.patch("jujulint.lint.Linter._log_with_header")
         linter.model.app_to_charm = self.check_spaces_example_app_charm_map
 
@@ -805,6 +806,7 @@ applications:
         )
 
     def test_check_spaces_enforce_endpoints(self, linter):
+        """Test that check spaces enforce endpoints."""
         linter.model.app_to_charm = self.check_spaces_example_app_charm_map
 
         # Run the space check with prometheus:target endpoint enforced.
@@ -824,6 +826,7 @@ applications:
         assert len(errors) == 2
 
     def test_check_spaces_enforce_relations(self, linter):
+        """Test that check spaces enforce relations."""
         linter.model.app_to_charm = self.check_spaces_example_app_charm_map
 
         # Run the space check with prometheus:target endpoint enforced.
@@ -845,6 +848,7 @@ applications:
         assert len(errors) == 2
 
     def test_check_spaces_ignore_endpoints(self, linter, mocker):
+        """Test that check spaces can ignore endpoints."""
         mock_log: mock.MagicMock = mocker.patch("jujulint.lint.Linter._log_with_header")
         linter.model.app_to_charm = self.check_spaces_example_app_charm_map
 
@@ -867,6 +871,7 @@ applications:
         assert mock_log.call_count == 0
 
     def test_check_spaces_ignore_relations(self, linter, mocker):
+        """Test that check spaces can ignore relations."""
         mock_log: mock.MagicMock = mocker.patch("jujulint.lint.Linter._log_with_header")
         linter.model.app_to_charm = self.check_spaces_example_app_charm_map
 
diff --git a/tox.ini b/tox.ini
index 843a442..40349c8 100644
--- a/tox.ini
+++ b/tox.ini
@@ -4,14 +4,15 @@ exclude =
     __pycache__,
     .tox,
     .eggs,
+    .venv,
     venv,
 max-line-length = 120
 max-complexity = 10
-ignore = C901
+ignore = D100, D103, C901
 
 [tox]
 skipsdist=True
-envlist = lintverbose, unit
+envlist = lint, unit, func
 skip_missing_interpreters = True
 
 [testenv]
@@ -19,9 +20,6 @@ basepython = python3.8
 deps =
     -r{toxinidir}/tests/requirements.txt
     -r{toxinidir}/requirements.txt
-commands =
-    lint: flake8 {toxinidir}
-    test: python3 -m unittest discover tests
 
 [testenv:unit]
 commands =
@@ -38,27 +36,39 @@ commands =
 setenv = PYTHONPATH={toxinidir}/lib
 
 [testenv:lint]
-commands = flake8 jujulint --format=html --htmldir=tests/report/lint/ --tee
-
-[testenv:lintverbose]
 commands =
-    flake8 {toxinidir}/jujulint {toxinidir}/tests
-    black --check {toxinidir}/jujulint {toxinidir}/tests
-    isort --check {toxinidir}/jujulint {toxinidir}/tests
+    flake8 --color always
+    black --check --diff --color .
+    isort --check-only --diff --color .
 
 
-[testenv:format-code]
+[testenv:reformat]
 commands =
-    black {toxinidir}/jujulint {toxinidir}/tests
-    isort {toxinidir}/jujulint {toxinidir}/tests
-
+    black .
+    isort .
 
-[testenv:lintjunit]
-commands = flake8 jujulint --format junit-xml --output-file=report/lint/junit.xml
+[testenv:func]
+#TODO(gabrielcocenza) add func tests
 
 [pytest]
-filterwarnings = 
+filterwarnings =
     ignore::DeprecationWarning
 
 [isort]
 profile = black
+skip_glob =
+    .eggs,
+    __pycache__,
+    .git,
+    .tox,
+    .venv,
+    .build,
+    venv,
+    dist,
+    mod,
+    build,
+
+[testenv:pre-commit]
+deps = pre-commit
+commands =
+    pre-commit run -a