Fix minor issues from code review, change how release-conf is loaded

.pylintrc

@@ -7,7 +7,7 @@ extension-pkg-whitelist=
 
 # Add files or directories to the blacklist. They should be base names, not
 # paths.
-ignore=CVS
+ignore=CVS,tests
 
 # Add files or directories matching the regex patterns to the blacklist. The
 # regex matches against base names, not paths.

README.md

@@ -7,7 +7,7 @@ repository and must be named `CHANGELOG.md`. Changelog for the new version must
 Everything between this heading and the heading for previous version will be pulled into the changelog.
 
 Alternatively, you can let the bot do the boring work, update `__version__` variable and fill changelog with commit messages from git log. 
-You can trigger this action by creating an issue and name the same as you would a release PR, e.g. `0.1.0 release`. 
+You can trigger this action by creating an issue and name it the same as you would a release PR, e.g. `0.1.0 release`. 
 All you have to do after that is merge the PR that the bot will make.
 
 A `release-conf.yaml` file is required. See [Configuration](#configuration) section for details.

release_bot/configuration.py

@@ -95,39 +95,38 @@ def load_configuration(self):
                 sys.exit(1)
         self.logger.debug(f"Loaded configuration for {self.repository_owner}/{self.repository_name}")
 
-    def load_release_conf(self, conf_dir):
+    def load_release_conf(self, conf):
         """
         Load items from release-conf.yaml
 
-        :param conf_dir: path to directory containing release-conf.yaml
+        :param conf: contents of release-conf.yaml
         :return dict with configuration
         """
-        conf_file_path = Path(conf_dir) / 'release-conf.yaml'
-        if not conf_file_path.is_file():
+        if not conf:
             self.logger.error("No release-conf.yaml found in "
                               f"{self.repository_owner}/{self.repository_name} repository root!\n"
                               "You have to add one for releasing to PyPi/Fedora")
             if self.REQUIRED_ITEMS['release-conf']:
                 sys.exit(1)
 
-        with conf_file_path.open() as conf_file:
-            parsed_conf = yaml.safe_load(conf_file) or {}
-            parsed_conf = {k: v for (k, v) in parsed_conf.items() if v}
-            for item in self.REQUIRED_ITEMS['release-conf']:
-                if item not in parsed_conf:
-                    self.logger.error(f"Item {item!r} is required in release-conf!")
-                    sys.exit(1)
-            for index, version in enumerate(parsed_conf.get('python_versions', [])):
-                parsed_conf['python_versions'][index] = int(version)
-            for index, branch in enumerate(parsed_conf.get('fedora_branches', [])):
-                parsed_conf['fedora_branches'][index] = str(branch)
-            if parsed_conf.get('fedora') and not self.fas_username:
-                self.logger.warning("Can't release to fedora if there is no FAS username, disabling")
-                parsed_conf['fedora'] = False
-            if parsed_conf.get('trigger_on_issue') and not self.github_username:
-                msg = "Can't trigger on issue if 'github_username' is not known, disabling"
-                self.logger.warning(msg)
-                parsed_conf['trigger_on_issue'] = False
+        parsed_conf = yaml.safe_load(conf) or {}
+        parsed_conf = {k: v for (k, v) in parsed_conf.items() if v}
+        for item in self.REQUIRED_ITEMS['release-conf']:
+            if item not in parsed_conf:
+                self.logger.error(f"Item {item!r} is required in release-conf!")
+                sys.exit(1)
+        for index, version in enumerate(parsed_conf.get('python_versions', [])):
+            parsed_conf['python_versions'][index] = int(version)
+        for index, branch in enumerate(parsed_conf.get('fedora_branches', [])):
+            parsed_conf['fedora_branches'][index] = str(branch)
+        if parsed_conf.get('fedora') and not self.fas_username:
+            self.logger.warning("Can't release to fedora if there is no FAS username, disabling")
+            parsed_conf['fedora'] = False
+        if parsed_conf.get('trigger_on_issue') and not self.github_username:
+            msg = "Can't trigger on issue if 'github_username' is not known, disabling"
+            self.logger.warning(msg)
+            parsed_conf['trigger_on_issue'] = False
+
         return parsed_conf
 
 

release_bot/github.py

@@ -235,7 +235,7 @@ def clone_repository(self):
         url = f'https://github.com/{self.conf.repository_owner}/{self.conf.repository_name}.git'
         return Git(url, self.conf)
 
-    def check_branch_existence(self, branch):
+    def branch_exists(self, branch):
         """
         Makes a call to github api to check if branch already exists
         :param branch: name of the branch
@@ -307,28 +307,28 @@ def make_release_pr(self, new_pr):
         repo = new_pr['repo']
         version = new_pr['version']
         branch = f'{version}-release'
-        if not self.check_branch_existence(branch):
-            try:
-                name, email = self.get_user_contact()
-                repo.set_credentials(name, email)
-                repo.set_credential_store()
-                changelog = repo.get_log_since_last_release(new_pr['previous_version'])
-                repo.checkout_new_branch(branch)
-                insert_in_changelog(f'{repo.repo_path}/CHANGELOG.md', new_pr['version'], changelog)
-                changed = look_for_version_files(repo.repo_path, new_pr['version'])
-                repo.add(['.'])
-                repo.commit(f'{version} release')
-                repo.push(branch)
-                if not self.check_if_pr_exists(f'{version} release'):
-                    new_pr['pr_url'] = self.make_pr(branch, f'{version}', changelog, changed)
-                    return True
-            except GitException as exc:
-                raise ReleaseException(exc)
-        else:
+        if self.branch_exists(branch):
             self.logger.warning(f'Branch {branch} already exists, aborting creating PR.')
+            return False
+        try:
+            name, email = self.get_user_contact()
+            repo.set_credentials(name, email)
+            repo.set_credential_store()
+            changelog = repo.get_log_since_last_release(new_pr['previous_version'])
+            repo.checkout_new_branch(branch)
+            insert_in_changelog(f'{repo.repo_path}/CHANGELOG.md', new_pr['version'], changelog)
+            changed = look_for_version_files(repo.repo_path, new_pr['version'])
+            repo.add(['.'])
+            repo.commit(f'{version} release')
+            repo.push(branch)
+            if not self.pr_exists(f'{version} release'):
+                new_pr['pr_url'] = self.make_pr(branch, f'{version}', changelog, changed)
+                return True
+        except GitException as exc:
+            raise ReleaseException(exc)
         return False
 
-    def check_if_pr_exists(self, name):
+    def pr_exists(self, name):
         """
         Makes a call to github api to check if PR already exists
         :param name: name of the PR
@@ -384,3 +384,25 @@ def close_issue(self, number):
             return True
         self.logger.error(f'Failed to close issue #{number}')
         return False
+
+    def get_configuration(self):
+        """
+        Fetches release-conf.yaml via Github API
+        :return: release-conf.yaml contents or False in case of error
+        """
+        url = (f"{self.API3_ENDPOINT}repos/{self.conf.repository_owner}/"
+               f"{self.conf.repository_name}/contents/release-conf.yaml")
+        self.logger.debug(f'Fetching release-conf.yaml')
+        response = requests.get(url=url, headers=self.headers)
+        if response.status_code != 200:
+            self.logger.error(f'Failed to fetch release-conf.yaml')
+            return False
+
+        parsed = response.json()
+        download_url = parsed['download_url']
+        response = requests.get(url=download_url)
+        if response.status_code != 200:
+            self.logger.error(f'Failed to fetch release-conf.yaml')
+            return False
+
+        return response.text

release_bot/releasebot.py

@@ -51,11 +51,14 @@ def cleanup(self):
         self.fedora.progress_log = []
 
     def load_release_conf(self):
+        """
+        Updates new_release with latest release-conf.yaml from repository
+        :return:
+        """
         # load release configuration from release-conf.yaml in repository
-        repo = self.github.clone_repository()
-        release_conf = self.conf.load_release_conf(repo.repo_path)
+        conf = self.github.get_configuration()
+        release_conf = self.conf.load_release_conf(conf)
         self.new_release.update(release_conf)
-        repo.cleanup()
 
     def find_open_release_issues(self):
         """
@@ -234,8 +237,8 @@ def run(self):
         while True:
             try:
                 if self.find_newest_release_pull_request():
-                    self.make_new_github_release()
                     self.load_release_conf()
+                    self.make_new_github_release()
                     # Try to do PyPi release regardless whether we just did github release
                     # for case that in previous iteration (of the 'while True' loop)
                     # we succeeded with github release, but failed with PyPi release

tests/test_load_release_conf.py

@@ -13,14 +13,15 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-from release_bot.configuration import configuration
 from pathlib import Path
 import pytest
 
+from release_bot.configuration import configuration
+
 
 class TestLoadReleaseConf:
 
-    def setup_method(self, method):
+    def setup_method(self):
         """ setup any state tied to the execution of the given method in a
         class.  setup_method is invoked for every test method of a class.
         """
@@ -33,17 +34,27 @@ def teardown_method(self, method):
         """
 
     @pytest.fixture
-    def empty_conf(self, tmpdir):
-        conf = Path(str(tmpdir))/"relase-conf.yaml"
-        conf.touch()
-        return str(tmpdir)
+    def empty_conf(self):
+        """
+        Emulates an empty configuration
+        :return:
+        """
+        return ""
 
     @pytest.fixture
-    def non_existing_conf(self, tmpdir):
-        return str(tmpdir)
+    def non_existing_conf(self):
+        """
+        Emulates missing configuration
+        :return:
+        """
+        return False
 
     @pytest.fixture
     def valid_new_release(self):
+        """
+        Emulates valid new_release dict
+        :return:
+        """
         new_release = {'version': '0.1.0',
                        'commitish': 'xxx',
                        'author_name': 'John Doe',
@@ -57,25 +68,28 @@ def valid_new_release(self):
         return new_release
 
     @pytest.fixture
-    def missing_items_conf(self, tmpdir):
-        conf_content = (Path(__file__).parent/"src/missing_items_conf.yaml").read_text()
-        conf = Path(str(tmpdir))/"release-conf.yaml"
-        conf.write_text(conf_content)
-        return str(tmpdir)
+    def missing_items_conf(self):
+        """
+        Emulates configuration with missing required items
+        :return:
+        """
+        return (Path(__file__).parent / "src/missing_items_conf.yaml").read_text()
 
     @pytest.fixture
-    def missing_author_conf(self, tmpdir):
-        conf_content = (Path(__file__).parent/"src/missing_author.yaml").read_text()
-        conf = Path(str(tmpdir))/"release-conf.yaml"
-        conf.write_text(conf_content)
-        return str(tmpdir)
+    def missing_author_conf(self):
+        """
+        Emulates configuration with missing author
+        :return:
+        """
+        return (Path(__file__).parent / "src/missing_author.yaml").read_text()
 
     @pytest.fixture
-    def valid_conf(self, tmpdir):
-        conf_content = (Path(__file__).parent/"src/release-conf.yaml").read_text()
-        conf = Path(str(tmpdir))/"release-conf.yaml"
-        conf.write_text(conf_content)
-        return str(tmpdir)
+    def valid_conf(self):
+        """
+        Emulates valid configuration
+        :return:
+        """
+        return (Path(__file__).parent / "src/release-conf.yaml").read_text()
 
     def test_empty_conf(self, empty_conf):
         # if there are any required items, this test must fail