diff --git a/requirements/base.in b/requirements/base.in index 632073e7..f08c3d4c 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -1,4 +1,5 @@ aiohttp +aiomemoizettl arrow cryptography>=2.6.1 dictdiffer diff --git a/requirements/base.txt b/requirements/base.txt index cf4100c2..76877469 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -1,4 +1,4 @@ -# SHA1:877ece2910765a8459ad805bf6d3866660799867 +# SHA1:e4fedf520da8ea79f254dd568a06441e6d3a665d # # This file is autogenerated by pip-compile-multi # To update, run: @@ -18,6 +18,9 @@ aiohttp==3.6.2 \ --hash=sha256:65f31b622af739a802ca6fd1a3076fd0ae523f8485c52924a89561ba10c49b48 \ --hash=sha256:ae55bac364c405caa23a4f2d6cfecc6a0daada500274ffca4a9230e7129eac59 \ --hash=sha256:b778ce0c909a2653741cb4b1ac7015b5c130ab9c897611df43ae6a58523cb965 +aiomemoizettl==0.0.3 \ + --hash=sha256:07a6becac60f6cd2604b9f2b73bcd9a50079a0b7b55e2a4e45b1eec5a3ea9659 \ + --hash=sha256:0a80d2dc765e545263f515363b6700ec8cf86fa3968b529f56390b28e34f743d arrow==0.15.4 \ --hash=sha256:01a16d8a93eddf86a29237f32ae36b29c27f047e79312eb4df5d55fd5a2b3183 \ --hash=sha256:e1a318a4c0b787833ae46302c02488b6eeef413c6a13324b3261ad320f21ec1e @@ -199,18 +202,18 @@ taskcluster-urls==11.0.0 \ --hash=sha256:2aceab7cf5b1948bc197f2e5e50c371aa48181ccd490b8bada00f1e3baf0c5cc \ --hash=sha256:74bd2110b5daaebcec5e1d287bf137b61cb8cf6b2d8f5f2b74183e32bc4e7c87 \ # via taskcluster -taskcluster==22.1.0 \ - --hash=sha256:0c3c657e5b9d4ab9028446df9cdeac8e16584574d35a94a589517cea1fd0d292 \ - --hash=sha256:d4c500b4fd9b030ef0898d81c9c6551f2beeebbdf687705817725b6095f60189 \ - --hash=sha256:da9857bdcb518a74ecfe0cbf34c7f4a047c13bfc8ee57b3a97e7d4edb6580667 +taskcluster==22.1.1 \ + --hash=sha256:07ef947d9b4fed32e4bc3e04b528f66a572944c6e2f907b8067c46b622067796 \ + --hash=sha256:0b87096327374303fa8ab1a8985917841bc9e704adeade6214f21ded7065dc9e \ + --hash=sha256:9221e19739f02bac4385fa3b1858028d92244b7f87d3b8e6dffe85e8fa118ff5 uritemplate==3.0.0 \ --hash=sha256:01c69f4fe8ed503b2951bef85d996a9d22434d2431584b5b107b2981ff416fbd \ --hash=sha256:1b9c467a940ce9fb9f50df819e8ddd14696f89b9a8cc87ac77952ba416e0a8fd \ --hash=sha256:c02643cebe23fc8adb5e6becffe201185bf06c40bda5c0b4028a93f1527d011d \ # via github3.py -urllib3==1.25.6 \ - --hash=sha256:3de946ffbed6e6746608990594d08faac602528ac7015ac28d33cee6a45b7398 \ - --hash=sha256:9a107b99a5393caf59c7aa3c1249c16e6879447533d0887f4336dde834c7be86 \ +urllib3==1.25.7 \ + --hash=sha256:a8a318824cc77d1fd4b2bec2ded92646630d7fe8619497b142c84a9e6f5a7293 \ + --hash=sha256:f3c5fd51747d450d4dcf6f923c81f78f811aab8205fda64b0aba34a4e48b0745 \ # via requests wheel==0.33.6 \ --hash=sha256:10c9da68765315ed98850f8e048347c3eb06dd81822dc2ab1d4fde9dc9702646 \ diff --git a/requirements/test.txt b/requirements/test.txt index e0484ae6..990422e8 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -95,9 +95,9 @@ pyflakes==2.1.1 \ --hash=sha256:17dbeb2e3f4d772725c777fabc446d5634d1038f234e77343108ce445ea69ce0 \ --hash=sha256:d976835886f8c5b31d47970ed689944a0262b5f3afa00a5a7b4dc81e5449f8a2 \ # via flake8 -pyparsing==2.4.4 \ - --hash=sha256:4acadc9a2b96c19fe00932a38ca63e601180c39a189a696abce1eaab641447e1 \ - --hash=sha256:61b5ed888beab19ddccab3478910e2076a6b5a0295dffc43021890e136edf764 \ +pyparsing==2.4.5 \ + --hash=sha256:20f995ecd72f2a1f4bf6b072b63b22e2eb457836601e76d6e5dfcd75436acc1f \ + --hash=sha256:4ca62001be367f01bd3e92ecbb79070272a9d4964dce6a48a82ff0b8bc7e683a \ # via packaging pytest-asyncio==0.10.0 \ --hash=sha256:9fac5100fd716cbecf6ef89233e8590a4ad61d729d1732e0a96b84182df1daaf \ @@ -111,9 +111,9 @@ pytest-mock==1.11.2 \ pytest-random-order==1.0.4 \ --hash=sha256:6b2159342a4c8c10855bc4fc6d65ee890fc614cb2b4ff688979b008a82a0ff52 \ --hash=sha256:72279a7f823969e18b10e438950f58330d17e0fcffb57cbd7929770cd687ecb2 -pytest==5.2.2 \ - --hash=sha256:27abc3fef618a01bebb1f0d6d303d2816a99aa87a5968ebc32fe971be91eb1e6 \ - --hash=sha256:58cee9e09242937e136dbb3dab466116ba20d6b7828c7620f23947f37eb4dae4 +pytest==5.2.3 \ + --hash=sha256:15837d2880cb94821087bc07476892ea740696b20e90288fd6c19e44b435abdb \ + --hash=sha256:b6cf7ad9064049ee486586b3a0ddd70dc5136c40e1147e7d286efd77ba66c5eb snowballstemmer==2.0.0 \ --hash=sha256:209f257d7533fdb3cb73bdbd24f436239ca3b2fa67d56f6ff88e86be08cc5ef0 \ --hash=sha256:df3bac3df4c2c01363f3dd2cfa78cce2840a79b9f1c2d2de9ce8d31683992f52 \ @@ -122,9 +122,9 @@ toml==0.10.0 \ --hash=sha256:229f81c57791a41d65e399fc06bf0848bab550a9dfd5ed66df18ce5f05e73d5c \ --hash=sha256:235682dd292d5899d361a811df37e04a8828a5b1da3115886b73cf81ebc9100e \ # via tox -tox==3.14.0 \ - --hash=sha256:0bc216b6a2e6afe764476b4a07edf2c1dab99ed82bb146a1130b2e828f5bff5e \ - --hash=sha256:c4f6b319c20ba4913dbfe71ebfd14ff95d1853c4231493608182f66e566ecfe1 +tox==3.14.1 \ + --hash=sha256:1d1368ac86e8332f79e2bcef9fefe2b077469f08449eadf0183759b34f3b2070 \ + --hash=sha256:bcfa3e40abc1e9b70607b56adfd976fe7dc8286ad56aab44e3151daca7d2d0d0 virtualenv==16.7.7 \ --hash=sha256:11cb4608930d5fd3afb545ecf8db83fa50e1f96fc4fca80c94b07d2c83146589 \ --hash=sha256:d257bb3773e48cac60e475a19b608996c73f4d333b3ba2e4e57d5ac6134e0136 diff --git a/scriptworker/github.py b/scriptworker/github.py index e97bc15c..dcaa2be8 100644 --- a/scriptworker/github.py +++ b/scriptworker/github.py @@ -3,6 +3,7 @@ import logging import re +from aiomemoizettl import memoize_ttl from github3 import GitHub from github3.exceptions import GitHubException @@ -15,7 +16,6 @@ ) _GIT_FULL_HASH_PATTERN = re.compile(r'^[0-9a-f]{40}$') -_branch_commits_cache = {} log = logging.getLogger(__name__) @@ -117,11 +117,6 @@ async def has_commit_landed_on_repository(self, context, revision): bool: True if the commit is present in one of the branches of the main repository """ - # Revision may be a tag name. `branch_commits` doesn't work on tags - if not _is_git_full_hash(revision): - revision = await self.get_tag_hash(tag_name=revision) - - repo = self._github_repository.html_url if any( vcs_rule.get('require_secret') for vcs_rule in context.config['trusted_vcs_rules'] @@ -132,21 +127,41 @@ async def has_commit_landed_on_repository(self, context, revision): "repositories, assume True") return True - url = '/'.join([repo.rstrip('/'), 'branch_commits', revision]) - from scriptworker.task import get_decision_task_id - cache_key = '{}-{}'.format(get_decision_task_id(context.task), url) - if cache_key in _branch_commits_cache: - html_text = _branch_commits_cache[cache_key] - else: - html_data = await retry_request(context, url) - html_text = html_data.strip() - _branch_commits_cache[cache_key] = html_text + # Revision may be a tag name. `branch_commits` doesn't work on tags + if not _is_git_full_hash(revision): + revision = await self.get_tag_hash(tag_name=revision) + + html_text = await _fetch_github_branch_commits_data( + context, self._github_repository.html_url, revision + ) + # https://github.com/{repo_owner}/{repo_name}/branch_commits/{revision} just returns some \n # when the commit hasn't landed on the origin repo. Otherwise, some HTML data is returned - it # represents the branches on which the given revision is present. return html_text != '' +# TODO Use memoize_ttl() as decorator once https://github.com/michalc/aiomemoizettl/issues/2 is done +async def _fetch_github_branch_commits_data_helper(context, repo_html_url, revision): + url = '/'.join((repo_html_url.rstrip('/'), 'branch_commits', revision)) + log.info('Cache does not exist for URL "{}" (in this context), fetching it...'.format(url)) + html_text = await retry_request(context, url) + return html_text.strip() + +# XXX memoize_ttl() uses all function parameters to create a key that stores its cache. +# This means new contexts cannot use the memoized value, even though they're calling the same +# repo and revision. jlorenzo tried to take the context out of the memoize_ttl() call, but +# whenever the cache is invalidated, request() doesn't work anymore because the session carried +# by the context has been long closed. +# Therefore, the defined TTL has 2 purposes: +# a. it memoizes calls for the time of a single cot_verify() run +# b. it clears the cache automatically, so we don't have to manually invalidate it. +_BRANCH_COMMITS_CACHE_TTL_IN_SECONDS = 10*60 # 10 minutes +_fetch_github_branch_commits_data = memoize_ttl( + _fetch_github_branch_commits_data_helper, get_ttl=lambda _: _BRANCH_COMMITS_CACHE_TTL_IN_SECONDS +) + + def is_github_url(url): """Tell if a given URL matches a Github one. diff --git a/scriptworker/test/test_github.py b/scriptworker/test/test_github.py index ba759ee6..b6ad7d7c 100644 --- a/scriptworker/test/test_github.py +++ b/scriptworker/test/test_github.py @@ -1,7 +1,9 @@ from types import SimpleNamespace +import asyncio import pytest +from copy import copy from unittest.mock import MagicMock, patch from . import mobile_rw_context, mpd_rw_context @@ -203,19 +205,34 @@ async def retry_request(_, url): @pytest.mark.asyncio -async def test_has_commit_landed_on_repository_cache(context, github_repository): - is_cached = False - async def retry_request(_, _url): - if is_cached: - assert False, "retry_request should not have been called." - return '\n' +async def test_has_commit_landed_on_repository_cache(context, mpd_context, github_repository): + global retry_request_call_count + retry_request_call_count = 0 + async def _counter(*args, **kwargs): + global retry_request_call_count + retry_request_call_count += 1 + return '' + + with patch('scriptworker.github.retry_request', _counter): + await asyncio.gather(*[ + github_repository.has_commit_landed_on_repository(context, "0129abcdef012345643456789abcdef012345678"), + github_repository.has_commit_landed_on_repository(context, "0129abcdef012345643456789abcdef012345678"), + github_repository.has_commit_landed_on_repository(context, "0129abcdef012345643456789abcdef012345678"), + github_repository.has_commit_landed_on_repository(context, "0129abcdef012345643456789abcdef012345678"), + ]) + # Even though all calls were fired at once, just a single call was made + assert retry_request_call_count == 1 + - with patch('scriptworker.github.retry_request', retry_request): - await github_repository.has_commit_landed_on_repository(context, "0129abcdef012345643456789abcdef012345678") - is_cached = True - await github_repository.has_commit_landed_on_repository(context, "0129abcdef012345643456789abcdef012345678") - is_cached = False await github_repository.has_commit_landed_on_repository(context, "456789abcdef0123456780129abcdef012345643") + # New commit means new request + assert retry_request_call_count == 2 + + different_context = copy(context) + different_context.task = {'taskGroupId': 'someOtherTaskId'} + await github_repository.has_commit_landed_on_repository(different_context, "456789abcdef0123456780129abcdef012345643") + # New context means new request too + assert retry_request_call_count == 3 @pytest.mark.parametrize('url, expected', (( diff --git a/setup.py b/setup.py index 6d669286..e812592f 100644 --- a/setup.py +++ b/setup.py @@ -18,6 +18,7 @@ reqs = [ "aiohttp>=3", + "aiomemoizettl", "arrow", "cryptography>=2.6.1", "dictdiffer",