diff --git a/.github/workflows/public-api-check.yml b/.github/workflows/public-api-check.yml new file mode 100644 index 00000000000..e151c40cf0c --- /dev/null +++ b/.github/workflows/public-api-check.yml @@ -0,0 +1,40 @@ +name: Public API check + +on: + pull_request: + types: [opened, synchronize, reopened, labeled, unlabeled] + branches: + - main + +jobs: + publicAPICheck: + + runs-on: ubuntu-latest + + if: "!contains(github.event.pull_request.labels.*.name, 'Skip Public API check')" + + steps: + - name: Checkout the repo + uses: actions/checkout@v2 + with: + fetch-depth: 0 + + - name: Checkout main + run: git checkout main + + - name: Pull origin + run: git pull --rebase=false origin main + + - name: Checkout pull request + run: git checkout ${{ github.event.pull_request.head.sha }} + + - name: Set up Python + uses: actions/setup-python@v2 + with: + python-version: 3.9 + + - name: Install tox + run: pip install -U tox-factor + + - name: Public API Check + run: tox -e public-symbols-check diff --git a/.pylintrc b/.pylintrc index 5daedb0139d..cf61fde78dd 100644 --- a/.pylintrc +++ b/.pylintrc @@ -73,8 +73,9 @@ disable=missing-docstring, exec-used, super-with-arguments, # temp-pylint-upgrade isinstance-second-argument-not-valid-type, # temp-pylint-upgrade - raise-missing-from, # temp-pylint-upgrade - unused-argument, # temp-pylint-upgrade + raise-missing-from, # temp-pylint-upgrade + unused-argument, # temp-pylint-upgrade + redefined-builtin, # Enable the message, report, category or checker with the given id(s). You can # either give multiple identifier separated by comma (,) or put this option diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9c6e72244de..e74979761a1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -69,6 +69,14 @@ You can run: Python version - `tox -e lint` to run lint checks on all code +We try to keep the amount of _public symbols_ in our code minimal. A public symbol is any Python identifier that does not start with an underscore. +Every public symbol is something that has to be kept in order to maintain backwards compatibility, so we try to have as few as possible. + +To check if your PR is adding public symbols, run `tox -e public-symbols-check`. This will always fail if public symbols are being added. The idea +behind this is that every PR that adds public symbols fails in CI, forcing reviewers to check the symbols to make sure they are strictly necessary. +If after checking them, it is considered that they are indeed necessary, the PR will be labeled with `Skip Public API check` so that this check is not +run. + See [`tox.ini`](https://github.com/open-telemetry/opentelemetry-python/blob/main/tox.ini) for more detail on available tox commands. diff --git a/scripts/public_symbols_checker.py b/scripts/public_symbols_checker.py new file mode 100644 index 00000000000..bdc530e95d3 --- /dev/null +++ b/scripts/public_symbols_checker.py @@ -0,0 +1,94 @@ +# Copyright The OpenTelemetry Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from difflib import unified_diff +from pathlib import Path +from re import match +from sys import exit + +from git import Repo +from git.db import GitDB + +repo = Repo(__file__, odbt=GitDB, search_parent_directories=True) + + +file_path_symbols = {} + + +def get_symbols(change_type, diff_lines_getter, prefix): + for diff_lines in ( + repo.commit("main") + .diff(repo.head.commit) + .iter_change_type(change_type) + ): + + b_file_path = diff_lines.b_blob.path + + if ( + Path(b_file_path).suffix != ".py" + or "opentelemetry" not in b_file_path + ): + continue + + for diff_line in diff_lines_getter(diff_lines): + matching_line = match( + r"{prefix}({symbol_re})\s=\s.+|" + r"{prefix}def\s({symbol_re})|" + r"{prefix}class\s({symbol_re})".format( + symbol_re=r"[a-zA-Z][_\w]+", prefix=prefix + ), + diff_line, + ) + + if matching_line is not None: + if b_file_path not in file_path_symbols.keys(): + file_path_symbols[b_file_path] = [] + + file_path_symbols[b_file_path].append( + next(filter(bool, matching_line.groups())) + ) + + +def a_diff_lines_getter(diff_lines): + return diff_lines.b_blob.data_stream.read().decode("utf-8").split("\n") + + +def m_diff_lines_getter(diff_lines): + return unified_diff( + diff_lines.a_blob.data_stream.read().decode("utf-8").split("\n"), + diff_lines.b_blob.data_stream.read().decode("utf-8").split("\n"), + ) + + +get_symbols("A", a_diff_lines_getter, r"") +get_symbols("M", m_diff_lines_getter, r"\+") + +if file_path_symbols: + print("The code in this branch adds the following public symbols:") + print() + for file_path, symbols in file_path_symbols.items(): + print("- {}".format(file_path)) + for symbol in symbols: + print("\t{}".format(symbol)) + print() + + print( + "Please make sure that all of them are strictly necessary, if not, " + "please consider prefixing them with an underscore to make them " + 'private. After that, please label this PR with "Skip Public API ' + 'check".' + ) + exit(1) +else: + print("The code in this branch will not add any public symbols") diff --git a/tox.ini b/tox.ini index f0ee49e971b..10d1f4925bd 100644 --- a/tox.ini +++ b/tox.ini @@ -33,7 +33,7 @@ envlist = ; opentelemetry-exporter-jaeger-proto-grpc py3{6,7,8,9}-test-exporter-jaeger-proto-grpc - + ; opentelemetry-exporter-jaeger-thrift py3{6,7,8,9}-test-exporter-jaeger-thrift @@ -78,6 +78,7 @@ envlist = mypy,mypyinstalled docs docker-tests + public-symbols-check [testenv] deps = @@ -186,6 +187,7 @@ deps = psutil readme_renderer httpretty + GitPython commands_pre = python -m pip install -e {toxinidir}/opentelemetry-api[test] @@ -268,3 +270,11 @@ commands = pytest {posargs} commands_post = docker-compose down -v + +[testenv:public-symbols-check] +basepython: python3.8 +recreate = True +deps = + GitPython +commands = + python {toxinidir}/scripts/public_symbols_checker.py