From b71fdbb0113bfe5c68f05742219def1522e7a90c Mon Sep 17 00:00:00 2001 From: David J Pugh <6003255+djpugh@users.noreply.github.com> Date: Sun, 24 Dec 2023 10:03:05 +0000 Subject: [PATCH] GitHub VCS Bugfixes (#21) * Bugfixes after initial functional usage * Fixing pyproject python troves * Updating readme template list --- docs/source/usage.md | 2 +- pyproject.toml | 2 +- src/nskit/mixer/components/license_file.py | 2 +- .../ingredients/docs/license.md.template | 2 +- .../python/ingredients/noxfile.py.template | 1 + .../ingredients/pyproject.toml.template | 2 +- .../python/ingredients/readme.md.template | 7 +-- src/nskit/vcs/providers/abstract.py | 7 +++ src/nskit/vcs/providers/github.py | 46 +++++++++++-------- src/nskit/vcs/repo.py | 9 +++- tests/functional/test_license_file.py | 16 +++---- .../test_components/test_license_file.py | 20 ++++---- tests/unit/test_vcs/test_repo.py | 16 +++---- 13 files changed, 77 insertions(+), 55 deletions(-) diff --git a/docs/source/usage.md b/docs/source/usage.md index 0eebcb0..8780cd6 100644 --- a/docs/source/usage.md +++ b/docs/source/usage.md @@ -442,7 +442,7 @@ This could be implemented as a staticmethod on the recipe object or similar, but You can also customise the environment initialisation if you need to override specifics of the configuration however this is not recommended as it can cause complex issues with the templates/handling. -The default implementation defines the loader to use ``_PkgResourcesTemplateLoader`` to allow for package resources type loading on the environment (see examples above), but other parameters could be configured/changed +The default implementation defines the loader to use ``_PkgResourcesTemplateLoader`` to allow for package resources type loading on the environment (see examples above), but other parameters could be configured/changed by setting the ``NSKIT_MIXER_JINJA_ENVIRONMENT_FACTORY`` environment variable to the name of the entrypoint. !!! warning diff --git a/pyproject.toml b/pyproject.toml index f949c4c..6b99a5e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -15,11 +15,11 @@ classifiers = [ "Environment :: Console", "Topic :: Software Development :: Libraries :: Python Modules", "Programming Language :: Python :: 3", - "Programming Language :: Python :: 3.7", "Programming Language :: Python :: 3.8", "Programming Language :: Python :: 3.9", "Programming Language :: Python :: 3.10", "Programming Language :: Python :: 3.11", + "Programming Language :: Python :: 3.12", "Intended Audience :: Developers", "License :: OSI Approved :: MIT License", "Operating System :: Microsoft :: Windows", diff --git a/src/nskit/mixer/components/license_file.py b/src/nskit/mixer/components/license_file.py index c2ca644..73fdaea 100644 --- a/src/nskit/mixer/components/license_file.py +++ b/src/nskit/mixer/components/license_file.py @@ -111,7 +111,7 @@ def get_license_content(context: Dict[str, Any]): LicenseOptionsEnum.BSD_3_Clause, LicenseOptionsEnum.MIT, ]: - content = content.replace('[year]', '{{license_year}}').replace('[fullname]', '{{name}} Developers') + content = content.replace('[year]', '{{license_year}}').replace('[fullname]', '{{repo.name}} Developers') context['license_year'] = context.get('license_year', date.today().year) return content diff --git a/src/nskit/recipes/python/ingredients/docs/license.md.template b/src/nskit/recipes/python/ingredients/docs/license.md.template index bf4cea2..bca9da8 100644 --- a/src/nskit/recipes/python/ingredients/docs/license.md.template +++ b/src/nskit/recipes/python/ingredients/docs/license.md.template @@ -1,7 +1,7 @@ {% if license %} # License -``{{repo.name}}`` is licensed under the {{license | upper }} License +``{{repo.name}}`` is licensed under the {{license.value | upper }} License {% raw %} ``` {% diff --git a/src/nskit/recipes/python/ingredients/noxfile.py.template b/src/nskit/recipes/python/ingredients/noxfile.py.template index 070ebbc..0ad65d2 100644 --- a/src/nskit/recipes/python/ingredients/noxfile.py.template +++ b/src/nskit/recipes/python/ingredients/noxfile.py.template @@ -1,6 +1,7 @@ import os from pathlib import Path from platform import platform, python_version +import sys if sys.version_info.major <=3 and sys.version_info.minor < 11: import tomli as tomllib # pyright: ignore [reportMissingImports] diff --git a/src/nskit/recipes/python/ingredients/pyproject.toml.template b/src/nskit/recipes/python/ingredients/pyproject.toml.template index d791812..beb73b3 100644 --- a/src/nskit/recipes/python/ingredients/pyproject.toml.template +++ b/src/nskit/recipes/python/ingredients/pyproject.toml.template @@ -15,11 +15,11 @@ classifiers = [ "Environment :: Console", "Topic :: Software Development :: Libraries :: Python Modules", "Programming Language :: Python :: 3", - "Programming Language :: Python :: 3.7", "Programming Language :: Python :: 3.8", "Programming Language :: Python :: 3.9", "Programming Language :: Python :: 3.10", "Programming Language :: Python :: 3.11", + "Programming Language :: Python :: 3.12", "Intended Audience :: Developers", "License :: OSI Approved :: MIT License", "Operating System :: Microsoft :: Windows", diff --git a/src/nskit/recipes/python/ingredients/readme.md.template b/src/nskit/recipes/python/ingredients/readme.md.template index a745637..08b1816 100644 --- a/src/nskit/recipes/python/ingredients/readme.md.template +++ b/src/nskit/recipes/python/ingredients/readme.md.template @@ -25,9 +25,10 @@ Tests can be added to the tests folder ### CI {% block CI %} There are a set of CI checks: - * lint: ``nox -t lint`` - * test: ``nox -t test`` - * build: ``nox -t build`` + +* lint: ``nox -t lint`` +* test: ``nox -t test`` +* build: ``nox -t build`` specific subfolders can be passed to the test tag in nox: ``nox -t test -- `` diff --git a/src/nskit/vcs/providers/abstract.py b/src/nskit/vcs/providers/abstract.py index 2cb5341..51461e8 100644 --- a/src/nskit/vcs/providers/abstract.py +++ b/src/nskit/vcs/providers/abstract.py @@ -20,6 +20,13 @@ def get_remote_url(self, repo_name: str) -> HttpUrl: """Get the remote url for a repo.""" raise NotImplementedError() + def get_clone_url(self, repo_name: str) -> HttpUrl: + """Get the clone URL. + + This defaults to the remote url unless specifically implemented. + """ + return self.get_remote_url(repo_name) + @abstractmethod def delete(self, repo_name: str): """Delete a repo.""" diff --git a/src/nskit/vcs/providers/github.py b/src/nskit/vcs/providers/github.py index 0205e58..61ca9d4 100644 --- a/src/nskit/vcs/providers/github.py +++ b/src/nskit/vcs/providers/github.py @@ -18,7 +18,7 @@ class GithubRepoSettings(BaseConfiguration): """Github Repo settings.""" private: bool = True - has_issue: Optional[bool] = None + has_issues: Optional[bool] = None has_wiki: Optional[bool] = None has_downloads: Optional[bool] = None has_projects: Optional[bool] = None @@ -36,7 +36,7 @@ class GithubSettings(VCSProviderSettings): """ model_config = SettingsConfigDict(env_prefix='GITHUB_', env_file='.env') interactive: bool = Field(False, description='Use Interactive Validation for token') - url: HttpUrl = "https://github.com" + url: HttpUrl = "https://api.github.com" organisation: Optional[str] = Field(None, description='Organisation to work in, otherwise uses the user for the token') token: SecretStr = Field(None, validate_default=True, description='Token to use for authentication, falls back to interactive device authentication if not provided') repo: GithubRepoSettings = GithubRepoSettings() @@ -69,7 +69,7 @@ class GithubRepoClient(RepoClient): def __init__(self, config: GithubSettings): """Initialise the client.""" self._config = config - self._github = GhApi(token=self._config.token.get_secret_value(), gh_host=self._config.url) + self._github = GhApi(token=self._config.token.get_secret_value(), gh_host=str(self._config.url).rstrip('/')) # If the organisation is set, we get it, and assume that the token is valid # Otherwise default to the user if self._config.organisation: @@ -77,31 +77,39 @@ def __init__(self, config: GithubSettings): self._github.orgs.get(self._config.organisation) self._org_type = GithubOrgType.org except HTTP404NotFoundError: - self._github.user.get_by_username(self._config.organisation) + self._github.users.get_by_username(self._config.organisation) self._org_type = GithubOrgType.user else: - self._config.organisation = self._github.get_authenticated()['login'] + self._config.organisation = self._github.users.get_authenticated()['login'] self._org_type = GithubOrgType.user def create(self, repo_name: str): """Create the repo in the user/organisation.""" - return self._github.repos.create_in_org( - self._config.organisation, - name=repo_name, - private=self._config.repo.private, - has_issues=self._config.repo.has_issues, - has_wiki=self._config.repo.has_wiki, - has_downloads=self._config.repo.has_downloads, - has_projects=self._config.repo.has_projects, - allow_squash_merge=self._config.repo.allow_squash_merge, - allow_merge_commit=self._config.repo.allow_merge_commit, - allow_rebase_merge=self._config.repo.allow_rebase_merge, - auto_init=self._config.repo.auto_init, - delete_branch_on_merge=self._config.repo.delete_branch_on_merge - ) + kwargs = { + 'name': repo_name, + 'private': self._config.repo.private, + 'has_issues': self._config.repo.has_issues, + 'has_wiki': self._config.repo.has_wiki, + 'has_downloads': self._config.repo.has_downloads, + 'has_projects': self._config.repo.has_projects, + 'allow_squash_merge': self._config.repo.allow_squash_merge, + 'allow_merge_commit': self._config.repo.allow_merge_commit, + 'allow_rebase_merge': self._config.repo.allow_rebase_merge, + 'auto_init': self._config.repo.auto_init, + 'delete_branch_on_merge': self._config.repo.delete_branch_on_merge + } + if self._org_type == GithubOrgType.org: + self._github.repos.create_in_org(self._config.organisation, **kwargs) + else: + self._github.repos.create_for_authenticated_user(**kwargs) def get_remote_url(self, repo_name: str) -> HttpUrl: """Get the remote url for the repo.""" + if self.check_exists(repo_name): + return self._github.repos.get(self._config.organisation, repo_name)['html_url'] + + def get_clone_url(self, repo_name: str) -> HttpUrl: + """Get the clone url for the repo.""" if self.check_exists(repo_name): return self._github.repos.get(self._config.organisation, repo_name)['clone_url'] diff --git a/src/nskit/vcs/repo.py b/src/nskit/vcs/repo.py index a63719d..219e0e6 100644 --- a/src/nskit/vcs/repo.py +++ b/src/nskit/vcs/repo.py @@ -99,6 +99,11 @@ def url(self): """Get the remote url.""" return self.provider_client.get_remote_url(self.name) + @property + def clone_url(self): + """Get the remote url.""" + return self.provider_client.get_clone_url(self.name) + def create(self): """Create the repo.""" if not self.exists: @@ -135,8 +140,8 @@ def clone(self): if not self.exists_locally: if not self.local_dir.parent.exists(): self.local_dir.parent.mkdir(exist_ok=True, parents=True) - if self.url: - self._git_repo_cls.clone_from(url=self.url, to_path=str(self.local_dir)) + if self.clone_url: + self._git_repo_cls.clone_from(url=self.clone_url, to_path=str(self.local_dir)) def pull(self, remote=DEFAULT_REMOTE): """Pull the repo from the remote (defaults to origin).""" diff --git a/tests/functional/test_license_file.py b/tests/functional/test_license_file.py index 6ea1a7f..e1698c9 100644 --- a/tests/functional/test_license_file.py +++ b/tests/functional/test_license_file.py @@ -24,7 +24,7 @@ def test_write_license(self): pre = [u for u in Path.cwd().glob('*')] license_file = LicenseFile() try: - resp = license_file.write(Path('.'), {'license':'mit', 'name': 'test_repo2'}) + resp = license_file.write(Path('.'), {'license':'mit', 'repo': {'name': 'test_repo2'}}) except HTTP403ForbiddenError: self.skip_if_rate_limited() post = [u for u in Path.cwd().glob('*')] @@ -40,7 +40,7 @@ def test_write_license(self): def test_dry_run_license(self): license_file = LicenseFile() try: - resp = license_file.dryrun(Path('.'), {'license':'mit', 'name': 'test_repo2'}) + resp = license_file.dryrun(Path('.'), {'license':'mit', 'repo': {'name': 'test_repo2'}}) except HTTP403ForbiddenError: self.skip_if_rate_limited() @@ -52,8 +52,8 @@ def test_validate_license_ok(self): with ChDir(): license_file = LicenseFile() try: - license_file.write(Path('.'), {'license':'mit', 'name': 'test_repo2'}) - missing, errors, ok = license_file.validate(Path('.'), {'license':'mit', 'name': 'test_repo2'}) + license_file.write(Path('.'), {'license':'mit', 'repo': {'name': 'test_repo2'}}) + missing, errors, ok = license_file.validate(Path('.'), {'license':'mit', 'repo': {'name': 'test_repo2'}}) except HTTP403ForbiddenError: self.skip_if_rate_limited() self.assertEqual(missing, []) @@ -64,7 +64,7 @@ def test_validate_license_missing(self): with ChDir(): license_file = LicenseFile() try: - missing, errors, ok = license_file.validate(Path('.'), {'license':'mit', 'name': 'test_repo2'}) + missing, errors, ok = license_file.validate(Path('.'), {'license':'mit', 'repo': {'name': 'test_repo2'}}) except HTTP403ForbiddenError: self.skip_if_rate_limited() self.assertEqual(missing, [Path('LICENSE')]) @@ -77,7 +77,7 @@ def test_validate_license_error(self): # License doesn't have the year fullname replacement try: license_file.write(Path('.'), {'license': 'mpl-2.0'}) - missing, errors, ok = license_file.validate(Path('.'), {'license':'mit', 'name': 'test_repo2'}) + missing, errors, ok = license_file.validate(Path('.'), {'license':'mit', 'repo': {'name': 'test_repo2'}}) except HTTP403ForbiddenError: self.skip_if_rate_limited() self.assertEqual(missing, []) @@ -87,7 +87,7 @@ def test_validate_license_error(self): def test_override_year(self): license_file = LicenseFile() try: - resp = license_file.dryrun(Path('.'), {'license':'mit', 'name': 'test_repo2', 'license_year': 1880}) + resp = license_file.dryrun(Path('.'), {'license':'mit', 'repo': {'name': 'test_repo2'}, 'license_year': 1880}) except HTTP403ForbiddenError: self.skip_if_rate_limited() @@ -100,7 +100,7 @@ def test_each_license_render_content(self): with self.subTest(license=license_name): # Test that it renders content try: - license_content = LicenseFile().render_content(context={'license': license_name, 'name': 'test_repo_name'}) + license_content = LicenseFile().render_content(context={'license': license_name, 'repo': {'name': 'test_repo_name'}}) except HTTP403ForbiddenError: self.skip_if_rate_limited() self.assertIsNotNone(license_content) diff --git a/tests/unit/test_mixer/test_components/test_license_file.py b/tests/unit/test_mixer/test_components/test_license_file.py index f3fe126..637ead8 100644 --- a/tests/unit/test_mixer/test_components/test_license_file.py +++ b/tests/unit/test_mixer/test_components/test_license_file.py @@ -50,12 +50,12 @@ class LicenseFileTestCase(unittest.TestCase): def test_get_license_content_cache(self, GhApi): # We need to clear the cache here on the test _get_license_content.cache_clear() - self.assertEqual(f'{date.today().year} test_repo Developers abc', LicenseFile().render_content({'license': 'mit', 'name': 'test_repo'})) + self.assertEqual(f'{date.today().year} test_repo Developers abc', LicenseFile().render_content({'license': 'mit', 'repo':{'name': 'test_repo'}})) GhApi.assert_called_once_with() GhApi().licenses.get.assert_called_once_with('mit') GhApi().licenses.get.reset_mock() GhApi.reset_mock() - self.assertEqual(f'{date.today().year} test_repo2 Developers abc', LicenseFile().render_content({'license': 'mit', 'name': 'test_repo2'})) + self.assertEqual(f'{date.today().year} test_repo2 Developers abc', LicenseFile().render_content({'license': 'mit', 'repo':{'name': 'test_repo2'}})) GhApi.assert_not_called() GhApi().licenses.get.assert_not_called() @@ -105,7 +105,7 @@ def test_write_license(self, GhApi): self.assertFalse(Path('UNLICENSE').exists()) pre = [u for u in Path.cwd().glob('*')] license_file = LicenseFile() - resp = license_file.write(Path('.'), {'license':'mit', 'name': 'test_repo2'}) + resp = license_file.write(Path('.'), {'license':'mit', 'repo':{'name': 'test_repo2'}}) post = [u for u in Path.cwd().glob('*')] self.assertTrue(Path('LICENSE').exists()) self.assertFalse(Path('COPYING').exists()) @@ -117,15 +117,15 @@ def test_write_license(self, GhApi): @mock_gh_api def test_dry_run_license(self, GhApi): license_file = LicenseFile() - resp = license_file.dryrun(Path('.'), {'license':'mit', 'name': 'test_repo2'}) + resp = license_file.dryrun(Path('.'), {'license':'mit', 'repo':{'name': 'test_repo2'}}) self.assertEqual(resp, {Path('LICENSE'): f'{date.today().year} test_repo2 Developers abc'}) @mock_gh_api def test_validate_license_ok(self, GhApi): with ChDir(): license_file = LicenseFile() - license_file.write(Path('.'), {'license':'mit', 'name': 'test_repo2'}) - missing, errors, ok = license_file.validate(Path('.'), {'license':'mit', 'name': 'test_repo2'}) + license_file.write(Path('.'), {'license':'mit', 'repo':{'name': 'test_repo2'}}) + missing, errors, ok = license_file.validate(Path('.'), {'license':'mit', 'repo':{'name': 'test_repo2'}}) self.assertEqual(missing, []) self.assertEqual(errors, []) self.assertEqual(ok, [Path('LICENSE')]) @@ -134,7 +134,7 @@ def test_validate_license_ok(self, GhApi): def test_validate_license_missing(self, GhApi): with ChDir(): license_file = LicenseFile() - missing, errors, ok = license_file.validate(Path('.'), {'license':'mit', 'name': 'test_repo2'}) + missing, errors, ok = license_file.validate(Path('.'), {'license':'mit', 'repo':{'name': 'test_repo2'}}) self.assertEqual(missing, [Path('LICENSE')]) self.assertEqual(errors, []) self.assertEqual(ok, []) @@ -145,7 +145,7 @@ def test_validate_license_error(self, GhApi): license_file = LicenseFile() # License doesn't have the year fullname replacement license_file.write(Path('.'), {'license': 'mpl-2.0'}) - missing, errors, ok = license_file.validate(Path('.'), {'license':'mit', 'name': 'test_repo2'}) + missing, errors, ok = license_file.validate(Path('.'), {'license':'mit', 'repo':{'name': 'test_repo2'}}) self.assertEqual(missing, []) self.assertEqual(errors, [Path('LICENSE')]) self.assertEqual(ok, []) @@ -153,7 +153,7 @@ def test_validate_license_error(self, GhApi): @mock_gh_api def test_override_year(self, GhApi): license_file = LicenseFile() - resp = license_file.dryrun(Path('.'), {'license':'mit', 'name': 'test_repo2', 'license_year': 2022}) + resp = license_file.dryrun(Path('.'), {'license':'mit', 'repo':{'name': 'test_repo2'}, 'license_year': 2022}) self.assertEqual(resp, {Path('LICENSE'): '2022 test_repo2 Developers abc'}) def test_license_filename(self): @@ -217,7 +217,7 @@ def test_each_license_render_content(self, GhApi): for license_name in LicenseOptionsEnum: with self.subTest(license=license_name): # Test that it renders content - license_content = LicenseFile().render_content(context={'license': license_name, 'name': 'test_repo_name'}) + license_content = LicenseFile().render_content(context={'license': license_name, 'repo':{'name': 'test_repo_name'}}) self.assertIsNotNone(license_content) self.assertNotIn('[year]', license_content) self.assertNotIn('[fullname]', license_content) diff --git a/tests/unit/test_vcs/test_repo.py b/tests/unit/test_vcs/test_repo.py index fcd7b78..d390c52 100644 --- a/tests/unit/test_vcs/test_repo.py +++ b/tests/unit/test_vcs/test_repo.py @@ -99,7 +99,7 @@ def test_create_not_existing_no_url(self): with ChDir(): cl = self._MockedRepoClientKls() cl.check_exists.return_value = False - cl.get_remote_url.return_value = None + cl.get_clone_url.return_value = None r = _Repo(name='test', provider_client=cl) r._git_repo_cls = self._MockedGitRepoKls r.create() @@ -111,7 +111,7 @@ def test_create_not_existing_url(self): with ChDir(): cl = self._MockedRepoClientKls() cl.check_exists.return_value = False - cl.get_remote_url.return_value = 'https://test.com' + cl.get_clone_url.return_value = 'https://test.com' r = _Repo(name='test', provider_client=cl) r._git_repo_cls = self._MockedGitRepoKls r.create() @@ -123,7 +123,7 @@ def test_create_existing_url(self): with ChDir(): cl = self._MockedRepoClientKls() cl.check_exists.return_value = True - cl.get_remote_url.return_value = 'https://test.com' + cl.get_clone_url.return_value = 'https://test.com' r = _Repo(name='test', provider_client=cl) r._git_repo_cls = self._MockedGitRepoKls r.create() @@ -169,7 +169,7 @@ def test_clone_exists_local(self): Path('test/').mkdir(parents=True) self.assertTrue(Path('test/').exists()) cl = self._MockedRepoClientKls() - cl.get_remote_url.return_value = 'https://test.com' + cl.get_clone_url.return_value = 'https://test.com' r = _Repo(name='test', provider_client=cl, local_dir=Path('test/abc')) r._git_repo_cls = self._MockedGitRepoKls r.clone() @@ -180,7 +180,7 @@ def test_clone_no_url(self): with ChDir(): self.assertFalse(Path('test/').exists()) cl = self._MockedRepoClientKls() - cl.get_remote_url.return_value = None + cl.get_clone_url.return_value = None r = _Repo(name='test', provider_client=cl, local_dir=Path('test/abc')) r._git_repo_cls = self._MockedGitRepoKls r.clone() @@ -191,7 +191,7 @@ def test_clone_url(self): with ChDir(): self.assertFalse(Path('test/').exists()) cl = self._MockedRepoClientKls() - cl.get_remote_url.return_value = 'https://test.com' + cl.get_clone_url.return_value = 'https://test.com' r = _Repo(name='test', provider_client=cl, local_dir=Path('test/abc')) r._git_repo_cls = self._MockedGitRepoKls r.clone() @@ -431,7 +431,7 @@ def test_download_namespaces(self): # Only uses clone and checkout, but we can't mock these on the BaseModel # Use tests from clone and checkout for _Repo cl = self._MockedRepoClientKls() - cl.get_remote_url.return_value = 'https://test.com' + cl.get_clone_url.return_value = 'https://test.com' r = NamespaceValidationRepo(name='abc', provider_client=cl, local_dir=Path('test/abc')) r._git_repo_cls = self._MockedGitRepoKls r._git_repo_cls.clone_from.assert_not_called() @@ -509,7 +509,7 @@ def test_load_namespace_validator_no_local(self): with open('test/abc/namespaces2.yaml', 'w') as f: f.write(nsv.model_dump_yaml()) cl = self._MockedRepoClientKls() - cl.get_remote_url.return_value = 'https://test.com' + cl.get_clone_url.return_value = 'https://test.com' r = NamespaceValidationRepo( namespaces_filename='namespaces2.yaml', name='abc',