From 17c3708f31631ce6a10c39ccc0ef3405feb94f04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Torres=20Cogollo?= Date: Mon, 22 Nov 2021 07:11:26 +0100 Subject: [PATCH] Bugfix: github_repo does not apply defaults on existing repos (#2386) * github_repo do not apply defaults on currently existing repos * Fixed sanity * Fixed doc defaults * Added changelog * Fix "or" statement and some formatting * Improve description change check * Added api_url parameter for unit tests and default values for private and description parameters * Added force_defaults parameter * Improved docs * Fixed doc anchors for force_defaults parameter * Update plugins/modules/source_control/github/github_repo.py Co-authored-by: Felix Fontein Co-authored-by: Felix Fontein --- ...386-github_repo-fix-idempotency-issues.yml | 2 + .../source_control/github/github_repo.py | 52 +++++++---- .../source_control/github/test_github_repo.py | 93 ++++++++++++++++--- 3 files changed, 119 insertions(+), 28 deletions(-) create mode 100644 changelogs/fragments/2386-github_repo-fix-idempotency-issues.yml diff --git a/changelogs/fragments/2386-github_repo-fix-idempotency-issues.yml b/changelogs/fragments/2386-github_repo-fix-idempotency-issues.yml new file mode 100644 index 0000000000..4d1133da0c --- /dev/null +++ b/changelogs/fragments/2386-github_repo-fix-idempotency-issues.yml @@ -0,0 +1,2 @@ +bugfixes: + - github_repo - ``private`` and ``description`` attributes should not be set to default values when the repo already exists (https://github.com/ansible-collections/community.general/pull/2386). diff --git a/plugins/modules/source_control/github/github_repo.py b/plugins/modules/source_control/github/github_repo.py index b5403c6a8d..1446e4abe9 100644 --- a/plugins/modules/source_control/github/github_repo.py +++ b/plugins/modules/source_control/github/github_repo.py @@ -42,16 +42,18 @@ options: description: description: - Description for the repository. + - Defaults to empty if I(force_defaults=true), which is the default in this module. + - Defaults to empty if I(force_defaults=false) when creating a new repository. - This is only used when I(state) is C(present). type: str - default: '' required: false private: description: - - Whether the new repository should be private or not. + - Whether the repository should be private or not. + - Defaults to C(false) if I(force_defaults=true), which is the default in this module. + - Defaults to C(false) if I(force_defaults=false) when creating a new repository. - This is only used when I(state) is C(present). type: bool - default: no required: false state: description: @@ -72,6 +74,14 @@ options: type: str default: 'https://api.github.com' version_added: "3.5.0" + force_defaults: + description: + - Overwrite current I(description) and I(private) attributes with defaults if set to C(true), which currently is the default. + - The default for this option will be deprecated in a future version of this collection, and eventually change to C(false). + type: bool + default: true + required: false + version_added: 4.1.0 requirements: - PyGithub>=1.54 notes: @@ -92,6 +102,7 @@ EXAMPLES = ''' description: "Just for fun" private: yes state: present + force_defaults: no register: result - name: Delete the repository @@ -117,7 +128,7 @@ import sys GITHUB_IMP_ERR = None try: - from github import Github, GithubException + from github import Github, GithubException, GithubObject from github.GithubException import UnknownObjectException HAS_GITHUB_PACKAGE = True except Exception: @@ -135,7 +146,7 @@ def authenticate(username=None, password=None, access_token=None, api_url=None): return Github(base_url=api_url, login_or_token=username, password=password) -def create_repo(gh, name, organization=None, private=False, description='', check_mode=False): +def create_repo(gh, name, organization=None, private=None, description=None, check_mode=False): result = dict( changed=False, repo=dict()) @@ -151,16 +162,21 @@ def create_repo(gh, name, organization=None, private=False, description='', chec except UnknownObjectException: if not check_mode: repo = target.create_repo( - name=name, private=private, description=description) + name=name, + private=GithubObject.NotSet if private is None else private, + description=GithubObject.NotSet if description is None else description, + ) result['repo'] = repo.raw_data result['changed'] = True changes = {} - if repo is None or repo.raw_data['private'] != private: - changes['private'] = private - if repo is None or repo.raw_data['description'] != description: - changes['description'] = description + if private is not None: + if repo is None or repo.raw_data['private'] != private: + changes['private'] = private + if description is not None: + if repo is None or repo.raw_data['description'] not in (description, description or None): + changes['description'] = description if changes: if not check_mode: @@ -193,6 +209,10 @@ def delete_repo(gh, name, organization=None, check_mode=False): def run_module(params, check_mode=False): + if params['force_defaults']: + params['description'] = params['description'] or '' + params['private'] = params['private'] or False + gh = authenticate( username=params['username'], password=params['password'], access_token=params['access_token'], api_url=params['api_url']) @@ -216,17 +236,17 @@ def run_module(params, check_mode=False): def main(): module_args = dict( - username=dict(type='str', required=False, default=None), - password=dict(type='str', required=False, default=None, no_log=True), - access_token=dict(type='str', required=False, - default=None, no_log=True), + username=dict(type='str'), + password=dict(type='str', no_log=True), + access_token=dict(type='str', no_log=True), name=dict(type='str', required=True), state=dict(type='str', required=False, default="present", choices=["present", "absent"]), organization=dict(type='str', required=False, default=None), - private=dict(type='bool', required=False, default=False), - description=dict(type='str', required=False, default=''), + private=dict(type='bool'), + description=dict(type='str'), api_url=dict(type='str', required=False, default='https://api.github.com'), + force_defaults=dict(type='bool', default=True), ) module = AnsibleModule( argument_spec=module_args, diff --git a/tests/unit/plugins/modules/source_control/github/test_github_repo.py b/tests/unit/plugins/modules/source_control/github/test_github_repo.py index b3e4f9027f..9c6a7037d1 100644 --- a/tests/unit/plugins/modules/source_control/github/test_github_repo.py +++ b/tests/unit/plugins/modules/source_control/github/test_github_repo.py @@ -71,6 +71,28 @@ def get_repo_mock(url, request): return response(200, content, headers, None, 5, request) +@urlmatch(netloc=r'api\.github\.com(:[0-9]+)?$', path=r'/repos/.*/.*', method="get") +def get_private_repo_mock(url, request): + match = re.search( + r"api\.github\.com(:[0-9]+)?/repos/(?P[^/]+)/(?P[^/]+)", request.url) + org = match.group("org") + repo = match.group("repo") + + # https://docs.github.com/en/rest/reference/repos#get-a-repository + headers = {'content-type': 'application/json'} + content = { + "name": repo, + "full_name": "{0}/{1}".format(org, repo), + "url": "https://api.github.com/repos/{0}/{1}".format(org, repo), + "private": True, + "description": "This your first repo!", + "default_branch": "master", + "allow_rebase_merge": True + } + content = json.dumps(content).encode("utf-8") + return response(200, content, headers, None, 5, request) + + @urlmatch(netloc=r'api\.github\.com(:[0-9]+)?$', path=r'/orgs/.*/repos', method="post") def create_new_org_repo_mock(url, request): match = re.search( @@ -83,8 +105,8 @@ def create_new_org_repo_mock(url, request): content = { "name": repo['name'], "full_name": "{0}/{1}".format(org, repo['name']), - "private": repo['private'], - "description": repo['description'] + "private": repo.get('private', False), + "description": repo.get('description') } content = json.dumps(content).encode("utf-8") return response(201, content, headers, None, 5, request) @@ -99,8 +121,8 @@ def create_new_user_repo_mock(url, request): content = { "name": repo['name'], "full_name": "{0}/{1}".format("octocat", repo['name']), - "private": repo['private'], - "description": repo['description'] + "private": repo.get('private', False), + "description": repo.get('description') } content = json.dumps(content).encode("utf-8") return response(201, content, headers, None, 5, request) @@ -120,8 +142,8 @@ def patch_repo_mock(url, request): "name": repo, "full_name": "{0}/{1}".format(org, repo), "url": "https://api.github.com/repos/{0}/{1}".format(org, repo), - "private": body['private'], - "description": body['description'], + "private": body.get('private', False), + "description": body.get('description'), "default_branch": "master", "allow_rebase_merge": True } @@ -160,11 +182,34 @@ class TestGithubRepo(unittest.TestCase): "description": "Just for fun", "private": False, "state": "present", - "api_url": "https://api.github.com" + "api_url": "https://api.github.com", + "force_defaults": False, }) self.assertEqual(result['changed'], True) self.assertEqual(result['repo']['private'], False) + self.assertEqual(result['repo']['description'], 'Just for fun') + + @with_httmock(get_orgs_mock) + @with_httmock(get_repo_notfound_mock) + @with_httmock(create_new_org_repo_mock) + def test_create_new_org_repo_incomplete(self): + result = github_repo.run_module({ + 'username': None, + 'password': None, + "access_token": "mytoken", + "organization": "MyOrganization", + "name": "myrepo", + "description": None, + "private": None, + "state": "present", + "api_url": "https://api.github.com", + "force_defaults": False, + }) + + self.assertEqual(result['changed'], True) + self.assertEqual(result['repo']['private'], False) + self.assertEqual(result['repo']['description'], None) @with_httmock(get_user_mock) @with_httmock(get_repo_notfound_mock) @@ -179,7 +224,8 @@ class TestGithubRepo(unittest.TestCase): "description": "Just for fun", "private": True, "state": "present", - "api_url": "https://api.github.com" + "api_url": "https://api.github.com", + "force_defaults": False, }) self.assertEqual(result['changed'], True) self.assertEqual(result['repo']['private'], True) @@ -197,11 +243,31 @@ class TestGithubRepo(unittest.TestCase): "description": "Just for fun", "private": True, "state": "present", - "api_url": "https://api.github.com" + "api_url": "https://api.github.com", + "force_defaults": False, }) self.assertEqual(result['changed'], True) self.assertEqual(result['repo']['private'], True) + @with_httmock(get_orgs_mock) + @with_httmock(get_private_repo_mock) + def test_idempotency_existing_org_private_repo(self): + result = github_repo.run_module({ + 'username': None, + 'password': None, + "access_token": "mytoken", + "organization": "MyOrganization", + "name": "myrepo", + "description": None, + "private": None, + "state": "present", + "api_url": "https://api.github.com", + "force_defaults": False, + }) + self.assertEqual(result['changed'], False) + self.assertEqual(result['repo']['private'], True) + self.assertEqual(result['repo']['description'], 'This your first repo!') + @with_httmock(get_orgs_mock) @with_httmock(get_repo_mock) @with_httmock(delete_repo_mock) @@ -215,7 +281,8 @@ class TestGithubRepo(unittest.TestCase): "description": "Just for fun", "private": False, "state": "absent", - "api_url": "https://api.github.com" + "api_url": "https://api.github.com", + "force_defaults": False, }) self.assertEqual(result['changed'], True) @@ -232,7 +299,8 @@ class TestGithubRepo(unittest.TestCase): "description": "Just for fun", "private": False, "state": "absent", - "api_url": "https://api.github.com" + "api_url": "https://api.github.com", + "force_defaults": False, }) self.assertEqual(result['changed'], True) @@ -249,7 +317,8 @@ class TestGithubRepo(unittest.TestCase): "description": "Just for fun", "private": True, "state": "absent", - "api_url": "https://api.github.com" + "api_url": "https://api.github.com", + "force_defaults": False, }) self.assertEqual(result['changed'], False)