OCA / oca-github-bot

The GitHub Bot of the Odoo Community Association (OCA)
MIT License
42 stars 61 forks source link

[REF] config and environment.sample files #109

Closed legalsylvain closed 4 years ago

legalsylvain commented 4 years ago

This PR provides a refactoring of config.py and environment.sample files.

When I discovered this project, I was quite lost between that two files because some things was in one or in the other, (or in both). So this PR is an attempt to make a little clean and remove duplicated code or comment.

Before this PR : 1) some default values was present in the environment.sample file, other in the config.py file, other in both.

2) Some description was present in both files like for BOT_TASKS value, and inconsistent between two files (keys was missing) : Description 1 : https://github.com/OCA/oca-github-bot/blob/master/src/oca_github_bot/config.py#L50 Description 2 : https://github.com/OCA/oca-github-bot/blob/master/environment.sample#L37

3) GITHUB_STATUS_IGNORED and GITHUB_CHECK_SUITES_IGNORED was hardcoded in config.py file, and so not configurable.

4) BOT_TASKS was enumerating the tasks to run. So during an upgrade of the lib, if the value was all, the new features was enabled by default (without changing the environment file) but if the value was not all the new features was disabled by default, that is not consistent. So BOT_TASKS is replace by BOT_TASKS_IGNORED assuming that most of the time, only some features are disabled and we want to have new features enabled by default.

Important Note : Once merged (if accepted ;-)), the custom environment file should be updated for the existing bots that are running. (At least the OCA one) adding the following keys. (if not still defined with a custom values.)

HTTP_PORT=8080
APPROVALS_REQUIRED=2
MIN_PR_AGE=5
GITHUB_STATUS_IGNORED=ci/runbot,codecov/project,codecov/patch,coverage/coveralls
GITHUB_CHECK_SUITES_IGNORED=Codecov
# If current BOT_TASKS value is "all"
BOT_TASKS_IGNORED=
# Otherwise, set the list of the tasks to skip
BOT_TASKS_IGNORED=task_1,task_2
legalsylvain commented 4 years ago

Hi @sbidoul. Thanks for your review. In retrospect, I think that my first proposal was a little too radical ! I took out 2 of the 5 commits. I refactored 2 commits

sbidoul commented 4 years ago

LGTM, thanks! Ideally this should be in two different PRs, if only to avoid conflating two independent topics in the same newsfragment.

legalsylvain commented 4 years ago

LGTM, thanks! Ideally this should be in two different PRs, if only to avoid conflating two independent topics in the same newsfragment.

feature 1) GITHUB_STATUS_IGNORED and GITHUB_CHECK_SUITES_IGNORED configurable --> available here : https://github.com/OCA/oca-github-bot/pull/111

feature 2) Add BOT_TASKS_IGNORED --> available here : https://github.com/OCA/oca-github-bot/pull/112