From 02c5c7e2b120c055b7db07d2ca9568d1ae25c399 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Wed, 2 Dec 2020 08:46:26 +0100 Subject: Fix leaderboard glitch caused by duplicate names We noticed that some entries on our leaderboard had an incorrect star count attached to their name. After a bit of digging, @HassanAbouelela discovered that this was caused by the use of the member's name as the key for the leaderboard dictionary: If different accounts used the same display name for the leaderboard, they'd be combined into one glitched score dict. The fix @HassanAbouelela wrote is to use the member id instead of the name as the key for the leaderboard. I've changed a few names here and there, but nothing major. This commit closes #536 --- bot/exts/christmas/advent_of_code/_helpers.py | 30 +++++++++++++++------------ 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/bot/exts/christmas/advent_of_code/_helpers.py b/bot/exts/christmas/advent_of_code/_helpers.py index f4a20955..e7eeedb2 100644 --- a/bot/exts/christmas/advent_of_code/_helpers.py +++ b/bot/exts/christmas/advent_of_code/_helpers.py @@ -50,7 +50,7 @@ EST = pytz.timezone("EST") # Create namedtuple that combines a participant's name and their completion # time for a specific star. We're going to use this later to order the results # for each star to compute the rank score. -_StarResult = collections.namedtuple("StarResult", "name completion_time") +StarResult = collections.namedtuple("StarResult", "member_id completion_time") def leaderboard_sorting_function(entry: typing.Tuple[str, dict]) -> typing.Tuple[int, int]: @@ -61,7 +61,7 @@ def leaderboard_sorting_function(entry: typing.Tuple[str, dict]) -> typing.Tuple secondary on the number of stars someone has completed. """ result = entry[1] - return result["score"], result["star_2_count"] + result["star_1_count"] + return result["score"], result["star_2"] + result["star_1"] def _parse_raw_leaderboard_data(raw_leaderboard_data: dict) -> dict: @@ -90,30 +90,34 @@ def _parse_raw_leaderboard_data(raw_leaderboard_data: dict) -> dict: # star view. We need that per star view to compute rank scores per star. for member in raw_leaderboard_data.values(): name = member["name"] if member["name"] else f"Anonymous #{member['id']}" - leaderboard[name] = {"score": 0, "star_1_count": 0, "star_2_count": 0} + member_id = member['id'] + leaderboard[member_id] = {"name": name, "score": 0, "star_1": 0, "star_2": 0} # Iterate over all days for this participant for day, stars in member["completion_day_level"].items(): # Iterate over the complete stars for this day for this participant for star, data in stars.items(): # Record completion of this star for this individual - leaderboard[name][f"star_{star}_count"] += 1 + leaderboard[member_id][f"star_{star}"] += 1 # Record completion datetime for this participant for this day/star completion_time = datetime.datetime.fromtimestamp(int(data['get_star_ts'])) star_results[(day, star)].append( - _StarResult(name=name, completion_time=completion_time) + StarResult(member_id=member_id, completion_time=completion_time) ) # Now that we have a transposed dataset that holds the completion time of all # participants per star, we can compute the rank-based scores each participant # should get for that star. max_score = len(leaderboard) - for(day, _star), results in star_results.items(): + for (day, _star), results in star_results.items(): + # If this day should not count in the ranking, skip it. if day in AdventOfCode.ignored_days: continue - for rank, star_result in enumerate(sorted(results, key=operator.itemgetter(1))): - leaderboard[star_result.name]["score"] += max_score - rank + + sorted_result = sorted(results, key=operator.attrgetter('completion_time')) + for rank, star_result in enumerate(sorted_result): + leaderboard[star_result.member_id]["score"] += max_score - rank # Since dictionaries now retain insertion order, let's use that sorted_leaderboard = dict( @@ -133,16 +137,16 @@ def _parse_raw_leaderboard_data(raw_leaderboard_data: dict) -> dict: return {"daily_stats": daily_stats, "leaderboard": sorted_leaderboard} -def _format_leaderboard(leaderboard: typing.Dict[str, int]) -> str: +def _format_leaderboard(leaderboard: typing.Dict[str, dict]) -> str: """Format the leaderboard using the AOC_TABLE_TEMPLATE.""" leaderboard_lines = [HEADER] - for rank, (name, results) in enumerate(leaderboard.items(), start=1): + for rank, data in enumerate(leaderboard.values(), start=1): leaderboard_lines.append( AOC_TABLE_TEMPLATE.format( rank=rank, - name=name, - score=str(results["score"]), - stars=f"({results['star_1_count']}, {results['star_2_count']})" + name=data["name"], + score=str(data["score"]), + stars=f"({data['star_1']}, {data['star_2']})" ) ) -- cgit v1.2.3 From 4d2f89991432639431968d352bde2cd747b9432c Mon Sep 17 00:00:00 2001 From: Rohan Date: Wed, 2 Dec 2020 21:58:09 +0530 Subject: Modify error handler check for locally handled errors. Error handler now checks if the error has the attribute "handled" for locally handled errors. --- bot/exts/evergreen/error_handler.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bot/exts/evergreen/error_handler.py b/bot/exts/evergreen/error_handler.py index 6e518435..b502dd4e 100644 --- a/bot/exts/evergreen/error_handler.py +++ b/bot/exts/evergreen/error_handler.py @@ -42,8 +42,8 @@ class CommandErrorHandler(commands.Cog): @commands.Cog.listener() async def on_command_error(self, ctx: commands.Context, error: commands.CommandError) -> None: """Activates when a command opens an error.""" - if hasattr(ctx.command, 'on_error'): - logging.debug("A command error occured but the command had it's own error handler.") + if hasattr(error, 'handled'): + logging.debug(f"Command {ctx.command} had its error already handled locally; ignoring.") return error = getattr(error, 'original', error) -- cgit v1.2.3 From cd24f9b4e70ebe654fa771238fca7e207e8cb0de Mon Sep 17 00:00:00 2001 From: Rohan Date: Fri, 4 Dec 2020 20:33:03 +0530 Subject: Check value of handled attribute on error in global error handler. --- bot/exts/evergreen/error_handler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/exts/evergreen/error_handler.py b/bot/exts/evergreen/error_handler.py index b502dd4e..99af1519 100644 --- a/bot/exts/evergreen/error_handler.py +++ b/bot/exts/evergreen/error_handler.py @@ -42,7 +42,7 @@ class CommandErrorHandler(commands.Cog): @commands.Cog.listener() async def on_command_error(self, ctx: commands.Context, error: commands.CommandError) -> None: """Activates when a command opens an error.""" - if hasattr(error, 'handled'): + if getattr(error, 'handled', False): logging.debug(f"Command {ctx.command} had its error already handled locally; ignoring.") return -- cgit v1.2.3 From 1aad543a8d4b3c7317fe98937144591d85ad3330 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Sat, 5 Dec 2020 00:21:04 +0100 Subject: Support a fallback session cookie in constants To mitigate problems due to expiring session cookies, I'm currently in the process of adding support for a fallback cookie. Basically, my Advent of Code account is a member of *all* leaderboards, which means that my cookie can be used to fetch all leaderboards as well. As my session cookie should not expire until after the event, it should not give us any issues. However, to avoid issues with issuing too many requests from one session, we should still make sure to set individual session values regardless of the mitigation. --- bot/constants.py | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/bot/constants.py b/bot/constants.py index e313e086..9e6db7a6 100644 --- a/bot/constants.py +++ b/bot/constants.py @@ -1,3 +1,4 @@ +import dataclasses import enum import logging from datetime import datetime @@ -29,11 +30,27 @@ __all__ = ( log = logging.getLogger(__name__) -class AdventOfCodeLeaderboard(NamedTuple): +@dataclasses.dataclass +class AdventOfCodeLeaderboard: id: str - session: str + _session: str join_code: str + # If we notice that the session for this board expired, we set + # this attribute to `True`. We will emit a Sentry error so we + # can handle it, but, in the meantime, we'll try using the + # fallback session to make sure the commands still work. + use_fallback_session: bool = False + + @property + def session(self) -> str: + """Return either the actual `session` cookie or the fallback cookie.""" + if self.use_fallback_session: + log.info(f"Returning fallback cookie for board `{self.id}`.") + return AdventOfCode.fallback_session + + return self._session + def _parse_aoc_leaderboard_env() -> Dict[str, AdventOfCodeLeaderboard]: """ @@ -61,6 +78,7 @@ class AdventOfCode: # Information for the several leaderboards we have leaderboards = _parse_aoc_leaderboard_env() staff_leaderboard_id = environ.get("AOC_STAFF_LEADERBOARD_ID", "") + fallback_session = environ.get("AOC_FALLBACK_SESSION", "") # Other Advent of Code constants ignored_days = [day for day in environ.get("AOC_IGNORED_DAYS", "").split(",")] -- cgit v1.2.3 From c21014abc7c74a72363daeac3714b9fa976247ce Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Sat, 5 Dec 2020 00:39:09 +0100 Subject: Mitigate session expiry by using fallback session Unfortunately, an expired session cookie wreaked havoc to our Advent of Code commands: All commands that relied on leaderboard data failed because we couldn't refresh our data and the cache had expired. To mitigate an expired session, I've added a fallback session feature that enables us to try again with a different session. While it will issue an error message to inform us to refresh the expired session cookie, it does mean that the functionality should continue to work in the mean time. The fallback session cookie is currently set to my session cookie, using an environment variable, `AOC_FALLBACK_SESSION`. It is important that the user connected to the session is a member of all boards and that it's a fresh session: We don't want our fallback to expire! At the same time, while a single fallback session works, the AoC website also does not like too many requests from a single user. That's why we'll still use a multi-session model under normal circumstances. To check for expired sessions, I've added a URL check: The Advent of Code website will silently redirect people with an expired session, issuing an 200: OK status as usual. The only way to really check for it is by comparing the final URL in the response object to the URL we set out to GET. I've added a custom exception to signal such an unexpected redirect. Finally, instead of having the commands just break, I've added an Exception signal that propagates back to the caller. The solution, with try-except, is a bit hacky and could benefit from an actual error handler, but I wanted to get things fixed first; polish can be added later. --- bot/exts/christmas/advent_of_code/_cog.py | 27 ++++++++--- bot/exts/christmas/advent_of_code/_helpers.py | 65 ++++++++++++++++++++++----- 2 files changed, 75 insertions(+), 17 deletions(-) diff --git a/bot/exts/christmas/advent_of_code/_cog.py b/bot/exts/christmas/advent_of_code/_cog.py index 2a1a776b..0bcd9f42 100644 --- a/bot/exts/christmas/advent_of_code/_cog.py +++ b/bot/exts/christmas/advent_of_code/_cog.py @@ -221,7 +221,11 @@ class AdventOfCode(commands.Cog): if AocConfig.staff_leaderboard_id and any(r.id == Roles.helpers for r in author.roles): join_code = AocConfig.leaderboards[AocConfig.staff_leaderboard_id].join_code else: - join_code = await _helpers.get_public_join_code(author) + try: + join_code = await _helpers.get_public_join_code(author) + except _helpers.FetchingLeaderboardFailed: + await ctx.send(":x: Failed to get join code! Notified maintainers.") + return if not join_code: log.error(f"Failed to get a join code for user {author} ({author.id})") @@ -256,7 +260,12 @@ class AdventOfCode(commands.Cog): async def aoc_leaderboard(self, ctx: commands.Context) -> None: """Get the current top scorers of the Python Discord Leaderboard.""" async with ctx.typing(): - leaderboard = await _helpers.fetch_leaderboard() + try: + leaderboard = await _helpers.fetch_leaderboard() + except _helpers.FetchingLeaderboardFailed: + await ctx.send(":x: Unable to fetch leaderboard!") + return + number_of_participants = leaderboard["number_of_participants"] top_count = min(AocConfig.leaderboard_displayed_members, number_of_participants) @@ -291,7 +300,11 @@ class AdventOfCode(commands.Cog): @override_in_channel(AOC_WHITELIST) async def private_leaderboard_daily_stats(self, ctx: commands.Context) -> None: """Send an embed with daily completion statistics for the Python Discord leaderboard.""" - leaderboard = await _helpers.fetch_leaderboard() + try: + leaderboard = await _helpers.fetch_leaderboard() + except _helpers.FetchingLeaderboardFailed: + await ctx.send(":x: Can't fetch leaderboard for stats right now!") + return # The daily stats are serialized as JSON as they have to be cached in Redis daily_stats = json.loads(leaderboard["daily_stats"]) @@ -323,8 +336,12 @@ class AdventOfCode(commands.Cog): many requests to the Advent of Code server. """ async with ctx.typing(): - await _helpers.fetch_leaderboard(invalidate_cache=True) - await ctx.send("\N{OK Hand Sign} Refreshed leaderboard cache!") + try: + await _helpers.fetch_leaderboard(invalidate_cache=True) + except _helpers.FetchingLeaderboardFailed: + await ctx.send(":x: Something went wrong while trying to refresh the cache!") + else: + await ctx.send("\N{OK Hand Sign} Refreshed leaderboard cache!") def cog_unload(self) -> None: """Cancel season-related tasks on cog unload.""" diff --git a/bot/exts/christmas/advent_of_code/_helpers.py b/bot/exts/christmas/advent_of_code/_helpers.py index e7eeedb2..d883c09f 100644 --- a/bot/exts/christmas/advent_of_code/_helpers.py +++ b/bot/exts/christmas/advent_of_code/_helpers.py @@ -53,6 +53,18 @@ EST = pytz.timezone("EST") StarResult = collections.namedtuple("StarResult", "member_id completion_time") +class UnexpectedRedirect(aiohttp.ClientError): + """Raised when an unexpected redirect was detected.""" + + +class UnexpectedResponseStatus(aiohttp.ClientError): + """Raised when an unexpected redirect was detected.""" + + +class FetchingLeaderboardFailed(Exception): + """Raised when one or more leaderboards could not be fetched at all.""" + + def leaderboard_sorting_function(entry: typing.Tuple[str, dict]) -> typing.Tuple[int, int]: """ Provide a sorting value for our leaderboard. @@ -153,6 +165,23 @@ def _format_leaderboard(leaderboard: typing.Dict[str, dict]) -> str: return "\n".join(leaderboard_lines) +async def _leaderboard_request(url: str, board: int, cookies: dict) -> typing.Optional[dict]: + """Make a leaderboard request using the specified session cookie.""" + async with aiohttp.request("GET", url, headers=AOC_REQUEST_HEADER, cookies=cookies) as resp: + # The Advent of Code website redirects silently with a 200 response if a + # session cookie has expired, is invalid, or was not provided. + if str(resp.url) != url: + log.error(f"Fetching leaderboard `{board}` failed! Check the session cookie.") + raise UnexpectedRedirect(f"redirected unexpectedly to {resp.url} for board `{board}`") + + # Every status other than `200` is unexpected, not only 400+ + if not resp.status == 200: + log.error(f"Unexpected response `{resp.status}` while fetching leaderboard `{board}`") + raise UnexpectedResponseStatus(f"status `{resp.status}`") + + return await resp.json() + + async def _fetch_leaderboard_data() -> typing.Dict[str, typing.Any]: """Fetch data for all leaderboards and return a pooled result.""" year = AdventOfCode.year @@ -165,22 +194,34 @@ async def _fetch_leaderboard_data() -> typing.Dict[str, typing.Any]: participants = {} for leaderboard in AdventOfCode.leaderboards.values(): leaderboard_url = AOC_API_URL.format(year=year, leaderboard_id=leaderboard.id) - cookies = {"session": leaderboard.session} - # We don't need to create a session if we're going to throw it away after each request - async with aiohttp.request( - "GET", leaderboard_url, headers=AOC_REQUEST_HEADER, cookies=cookies - ) as resp: - if resp.status == 200: - raw_data = await resp.json() - - # Get the participants and store their current count + # Two attempts, one with the original session cookie and one with the fallback session + for attempt in range(1, 3): + log.info(f"Attempting to fetch leaderboard `{leaderboard.id}` ({attempt}/2)") + cookies = {"session": leaderboard.session} + try: + raw_data = await _leaderboard_request(leaderboard_url, leaderboard.id, cookies) + except UnexpectedRedirect: + if cookies["session"] == AdventOfCode.fallback_session: + log.error("It seems like the fallback cookie has expired!") + raise FetchingLeaderboardFailed from None + + # If we're here, it means that the original session did not + # work. Let's fall back to the fallback session. + leaderboard.use_fallback_session = True + continue + except aiohttp.ClientError: + # Don't retry, something unexpected is wrong and it may not be the session. + raise FetchingLeaderboardFailed from None + else: + # Get the participants and store their current count. board_participants = raw_data["members"] await _caches.leaderboard_counts.set(leaderboard.id, len(board_participants)) participants.update(board_participants) - else: - log.warning(f"Fetching data failed for leaderboard `{leaderboard.id}`") - resp.raise_for_status() + break + else: + log.error(f"reached 'unreachable' state while fetching board `{leaderboard.id}`.") + raise FetchingLeaderboardFailed log.info(f"Fetched leaderboard information for {len(participants)} participants") return participants -- cgit v1.2.3 From a2d85eb6b5fc68b687741f23bb5061802f2f88cb Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Sat, 5 Dec 2020 13:01:55 +0100 Subject: Use custom status embeds for workflow runs This commit introduces enhanced status embeds for workflow runs that give more information about run that just ended. An added advantage is that we can disable the default "give me everything"-setting of GitHub and fine tune when we want to send embeds. This allows us to only send an embed for the `lint->build` sequence when the sequence ends (e.g. in the end when done or after an intermediate step due to failure/cancellation). --- .github/workflows/build.yaml | 31 +++++++++++++++++++++++++++++++ .github/workflows/lint.yaml | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index b0c03139..64c272cf 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -75,3 +75,34 @@ jobs: kubernetes/sir-lancebot/deployment.yaml images: 'ghcr.io/python-discord/sir-lancebot:${{ steps.sha_tag.outputs.tag }}' kubectl-version: 'latest' + + # Send an informational status embed to Discord instead of the + # standard embeds that Discord sends. This embed will contain + # more information and we can fine tune when we actually want + # to send an embed. + - name: GitHub Actions Status Embed for Discord + # This is the last step in the lint-build sequence, so always send + # an embed, regardless of success, failure or cancelled status. + if: always() + uses: SebastiaanZ/github-status-embed-for-discord@v0.1.1 + with: + # Our GitHub Actions webhook + webhook_id: '784184528997842985' + webhook_token: ${{ secrets.GHA_WEBHOOK_TOKEN }} + + # Workflow information + workflow_name: ${{ github.workflow }} + run_id: ${{ github.run_id }} + run_number: ${{ github.run_number }} + status: ${{ job.status }} + actor: ${{ github.actor }} + repository: ${{ github.repository }} + ref: ${{ github.ref }} + sha: ${{ github.sha }} + + # Optional PR-information. These values will be "null" if + # the event trigger was not PR-related. + pr_author_login: ${{ github.event.pull_request.user.login }} + pr_number: ${{ github.event.pull_request.number }} + pr_title: ${{ github.event.pull_request.title }} + pr_source: ${{ github.event.pull_request.head.label }} diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml index 063f406c..8dd93773 100644 --- a/.github/workflows/lint.yaml +++ b/.github/workflows/lint.yaml @@ -91,3 +91,35 @@ jobs: - name: Run flake8 run: "flake8 \ --format='::error file=%(path)s,line=%(row)d,col=%(col)d::[flake8] %(code)s: %(text)s'" + + # Send an informational status embed to Discord instead of the + # standard embeds that Discord sends. This embed will contain + # more information and we can fine tune when we actually want + # to send an embed. + - name: GitHub Actions Status Embed for Discord + # For a `pull_request` we always want to send a status embed + # here. For a push event, only when this workflow is the last + # in lint->build sequence because it failed or was cancelled. + if: github.event_name == 'pull_request' || cancelled() || failure() + uses: SebastiaanZ/github-status-embed-for-discord@v0.1.1 + with: + # Our GitHub Actions webhook + webhook_id: '784184528997842985' + webhook_token: ${{ secrets.GHA_WEBHOOK_TOKEN }} + + # Workflow information + workflow_name: ${{ github.workflow }} + run_id: ${{ github.run_id }} + run_number: ${{ github.run_number }} + status: ${{ job.status }} + actor: ${{ github.actor }} + repository: ${{ github.repository }} + ref: ${{ github.ref }} + sha: ${{ github.sha }} + + # Optional PR-information. These values will be "null" if + # the event trigger was not PR-related. + pr_author_login: ${{ github.event.pull_request.user.login }} + pr_number: ${{ github.event.pull_request.number }} + pr_title: ${{ github.event.pull_request.title }} + pr_source: ${{ github.event.pull_request.head.label }} -- cgit v1.2.3 From 8d3f326c7971a2b3547ea220272d9465b49986db Mon Sep 17 00:00:00 2001 From: Joe Banks Date: Sun, 6 Dec 2020 18:42:16 +0000 Subject: Delete review check GitHub Action --- .github/workflows/review-check.yaml | 166 ------------------------------------ 1 file changed, 166 deletions(-) delete mode 100644 .github/workflows/review-check.yaml diff --git a/.github/workflows/review-check.yaml b/.github/workflows/review-check.yaml deleted file mode 100644 index 3e45a4b5..00000000 --- a/.github/workflows/review-check.yaml +++ /dev/null @@ -1,166 +0,0 @@ -name: Review Check - -# This workflow needs to trigger in two situations: -# -# 1. When a pull request is opened, reopened, or synchronized (new commit) -# This is accomplished using the `pull_request_target` event that triggers in -# precisely those situations by default. I've opted for `pull_request_target` -# as we don't need to have access to the PR's code and it's safer to make the -# secrets we need available to the workflow compared to `pull_request`. -# -# The reason we need to run the workflow for this event is because we need to -# make sure that our check is part of the check suite for the current commit. -# -# 2. When a review is added or dismissed. -# Whenever reviews are submitted or dismissed, the number of Core Developer -# approvals may obviously change. -# -# --- -# -# Unfortunately, having two different event triggers means that can't let -# this workflow fail on its own, as GitHub actions registers a separate check -# run result per event trigger. As both triggers need to share the success/fail -# state, we get around that by registering a custom "status". -on: - pull_request_review: - types: - - submitted - - dismissed - pull_request_target: - - -jobs: - review-check: - name: Check Core Dev Reviews - runs-on: ubuntu-latest - - steps: - # Fetch the latest Opinionated reviews from users with write - # access. We can't narrow it down using a specific team here - # yet, so we'll do that later. - - uses: octokit/graphql-action@v2.x - id: reviews - with: - query: | - query ($repository: String!, $pr: Int!) { - repository(owner: "python-discord", name: $repository) { - pullRequest(number: $pr) { - latestOpinionatedReviews(last: 100, writersOnly: true) { - nodes{ - author{ - login - } - state - } - } - } - } - } - repository: ${{ github.event.repository.name }} - pr: ${{ github.event.pull_request.number }} - env: - GITHUB_TOKEN: ${{ secrets.REPO_TOKEN }} - - # Fetch the members of the Core Developers team so we can - # check if any of them actually approved this PR. - - uses: octokit/graphql-action@v2.x - id: core_developers - with: - query: | - query { - organization(login: "python-discord") { - team(slug: "core-developers") { - members(first: 100) { - nodes { - login - } - } - } - } - } - env: - GITHUB_TOKEN: ${{ secrets.TEAM_TOKEN }} - - # I've opted for a Python script, as that's what most of us - # are familiar with. We do need to setup Python for that. - - name: Setup python - id: python - uses: actions/setup-python@v2 - with: - python-version: '3.9' - - # This is a small, inline Python script that looks for the - # intersection between approving reviewers and the core dev - # team. If that intersection exists, we have at least one - # approving Core Developer. - # - # I've opted to keep this inline as it's relatively small - # and this workflow will be added to multiple repositories. - - name: Check for Accepting Core Developers - id: core_dev_reviews - run: | - python -c 'import json - reviews = json.loads("""${{ steps.reviews.outputs.data }}""") - reviewers = { - review["author"]["login"] - for review in reviews["repository"]["pullRequest"]["latestOpinionatedReviews"]["nodes"] - if review["state"] == "APPROVED" - } - core_devs = json.loads("""${{ steps.core_developers.outputs.data }}""") - core_devs = { - member["login"] for member in core_devs["organization"]["team"]["members"]["nodes"] - } - approving_core_devs = reviewers & core_devs - approval_check = "success" if approving_core_devs else "failure" - print(f"::set-output name=approval_check::{approval_check}") - ' - - # This step registers a a new status for the head commit of the pull - # request. If a status with the same context and description already - # exists, it will be overwritten. The reason we have to do this is - # because workflows run for the separate `pull_request_target` and - #`pull_request_review` events need to share a single result state. - - name: Add Core Dev Approval status check - uses: octokit/request-action@v2.x - with: - route: POST /repos/:repository/statuses/:sha - repository: ${{ github.repository }} - sha: ${{ github.event.pull_request.head.sha }} - state: ${{ steps.core_dev_reviews.outputs.approval_check }} - description: At least one core developer needs to approve this PR - context: Core Dev Approval - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - - # If we have at least one Core Developer approval, this step - # removes the 'waiting for core dev approval' label if it's - # still present for the PR. - - name: Remove "waiting for core dev approval" if a core dev approved this PR - if: >- - steps.core_dev_reviews.outputs.approval_check == 'success' && - contains(github.event.pull_request.labels.*.name, 'waiting for core dev approval') - uses: octokit/request-action@v2.x - with: - route: DELETE /repos/:repository/issues/:number/labels/:label - repository: ${{ github.repository }} - number: ${{ github.event.pull_request.number }} - label: needs core dev approval - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - - # If we have do not have one Core Developer approval, this step - # adds the 'waiting for core dev approval' label if it's not - # already present for the PR. - - name: Add "waiting for core dev approval" if no core dev has approved yet - if: >- - steps.core_dev_reviews.outputs.approval_check == 'failure' && - !contains(github.event.pull_request.labels.*.name, 'waiting for core dev approval') - uses: octokit/request-action@v2.x - with: - route: POST /repos/:repository/issues/:number/labels - repository: ${{ github.repository }} - number: ${{ github.event.pull_request.number }} - labels: | - - needs core dev approval - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} -- cgit v1.2.3 From 186159d1af5cd87043586d4595b7647fbf1d6056 Mon Sep 17 00:00:00 2001 From: Joe Banks Date: Sun, 6 Dec 2020 18:44:06 +0000 Subject: Create review-policy.yml --- .github/review-policy.yml | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .github/review-policy.yml diff --git a/.github/review-policy.yml b/.github/review-policy.yml new file mode 100644 index 00000000..421b30f8 --- /dev/null +++ b/.github/review-policy.yml @@ -0,0 +1,3 @@ +remote: python-discord/.github +path: review-policies/core-developers.yml +ref: main -- cgit v1.2.3 From cd20acfee1e36351e561ba7c410dc8fac7725572 Mon Sep 17 00:00:00 2001 From: Rohan Date: Mon, 7 Dec 2020 01:16:29 +0530 Subject: Modify snakes_cog error handler. local error handler no longer checks for BadArgument error and the attribute handled will be set to True on the error if an OSError occurs. --- bot/exts/evergreen/snakes/_snakes_cog.py | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/bot/exts/evergreen/snakes/_snakes_cog.py b/bot/exts/evergreen/snakes/_snakes_cog.py index 70bb0e73..2e88c146 100644 --- a/bot/exts/evergreen/snakes/_snakes_cog.py +++ b/bot/exts/evergreen/snakes/_snakes_cog.py @@ -15,7 +15,7 @@ import aiohttp import async_timeout from PIL import Image, ImageDraw, ImageFont from discord import Colour, Embed, File, Member, Message, Reaction -from discord.ext.commands import BadArgument, Bot, Cog, CommandError, Context, bot_has_permissions, group +from discord.ext.commands import Bot, Cog, CommandError, Context, bot_has_permissions, group from bot.constants import ERROR_REPLIES, Tokens from bot.exts.evergreen.snakes import _utils as utils @@ -1131,21 +1131,11 @@ class Snakes(Cog): @video_command.error async def command_error(self, ctx: Context, error: CommandError) -> None: """Local error handler for the Snake Cog.""" - embed = Embed() - embed.colour = Colour.red() - - if isinstance(error, BadArgument): - embed.description = str(error) - embed.title = random.choice(ERROR_REPLIES) - - elif isinstance(error, OSError): + if isinstance(error, OSError): + error.handled = True + embed = Embed() + embed.colour = Colour.red() log.error(f"snake_card encountered an OSError: {error} ({error.original})") embed.description = "Could not generate the snake card! Please try again." embed.title = random.choice(ERROR_REPLIES) - - else: - log.error(f"Unhandled tag command error: {error} ({error.original})") - return - - await ctx.send(embed=embed) - # endregion + await ctx.send(embed=embed) -- cgit v1.2.3 From 7cd3e74d46927fb667cb1e8c336be960e86647a1 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Wed, 9 Dec 2020 23:06:30 +0100 Subject: Use workflow_run to send status embed to Discord I've changed the way we send status embeds to make it work for PRs made from forks without potentially exposing secrets. Instead of using the initial workflows to send the embed, I've created a `workflow_run` workflow that always runs in the context of the base repository. And added benefit is that we don't have to add the status embed step to two separate workflows. --- .github/workflows/build.yaml | 31 ---------------- .github/workflows/lint.yaml | 32 ----------------- .github/workflows/status_embed.yaml | 71 +++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 63 deletions(-) create mode 100644 .github/workflows/status_embed.yaml diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 64c272cf..b0c03139 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -75,34 +75,3 @@ jobs: kubernetes/sir-lancebot/deployment.yaml images: 'ghcr.io/python-discord/sir-lancebot:${{ steps.sha_tag.outputs.tag }}' kubectl-version: 'latest' - - # Send an informational status embed to Discord instead of the - # standard embeds that Discord sends. This embed will contain - # more information and we can fine tune when we actually want - # to send an embed. - - name: GitHub Actions Status Embed for Discord - # This is the last step in the lint-build sequence, so always send - # an embed, regardless of success, failure or cancelled status. - if: always() - uses: SebastiaanZ/github-status-embed-for-discord@v0.1.1 - with: - # Our GitHub Actions webhook - webhook_id: '784184528997842985' - webhook_token: ${{ secrets.GHA_WEBHOOK_TOKEN }} - - # Workflow information - workflow_name: ${{ github.workflow }} - run_id: ${{ github.run_id }} - run_number: ${{ github.run_number }} - status: ${{ job.status }} - actor: ${{ github.actor }} - repository: ${{ github.repository }} - ref: ${{ github.ref }} - sha: ${{ github.sha }} - - # Optional PR-information. These values will be "null" if - # the event trigger was not PR-related. - pr_author_login: ${{ github.event.pull_request.user.login }} - pr_number: ${{ github.event.pull_request.number }} - pr_title: ${{ github.event.pull_request.title }} - pr_source: ${{ github.event.pull_request.head.label }} diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml index 8dd93773..063f406c 100644 --- a/.github/workflows/lint.yaml +++ b/.github/workflows/lint.yaml @@ -91,35 +91,3 @@ jobs: - name: Run flake8 run: "flake8 \ --format='::error file=%(path)s,line=%(row)d,col=%(col)d::[flake8] %(code)s: %(text)s'" - - # Send an informational status embed to Discord instead of the - # standard embeds that Discord sends. This embed will contain - # more information and we can fine tune when we actually want - # to send an embed. - - name: GitHub Actions Status Embed for Discord - # For a `pull_request` we always want to send a status embed - # here. For a push event, only when this workflow is the last - # in lint->build sequence because it failed or was cancelled. - if: github.event_name == 'pull_request' || cancelled() || failure() - uses: SebastiaanZ/github-status-embed-for-discord@v0.1.1 - with: - # Our GitHub Actions webhook - webhook_id: '784184528997842985' - webhook_token: ${{ secrets.GHA_WEBHOOK_TOKEN }} - - # Workflow information - workflow_name: ${{ github.workflow }} - run_id: ${{ github.run_id }} - run_number: ${{ github.run_number }} - status: ${{ job.status }} - actor: ${{ github.actor }} - repository: ${{ github.repository }} - ref: ${{ github.ref }} - sha: ${{ github.sha }} - - # Optional PR-information. These values will be "null" if - # the event trigger was not PR-related. - pr_author_login: ${{ github.event.pull_request.user.login }} - pr_number: ${{ github.event.pull_request.number }} - pr_title: ${{ github.event.pull_request.title }} - pr_source: ${{ github.event.pull_request.head.label }} diff --git a/.github/workflows/status_embed.yaml b/.github/workflows/status_embed.yaml new file mode 100644 index 00000000..1d175fb9 --- /dev/null +++ b/.github/workflows/status_embed.yaml @@ -0,0 +1,71 @@ +name: Status Embed + +on: + workflow_run: + workflows: + - Lint + - Build + types: + - completed + +jobs: + status_embed: + # We send the embed in the following situations: + # - Always after the `Build` workflow, as it runs at the + # end of our workflow sequence regardless of status. + # - Always for the `pull_request` event, as it only + # runs one workflow. + # - Always run for non-success workflows, as they + # terminate the workflow sequence. + if: >- + github.event.workflow_run.name == 'Build' || + github.event.workflow_run.event == 'pull_request' || + github.event.workflow_run.conclusion != 'success' + name: Send Status Embed to Discord + runs-on: ubuntu-latest + + steps: + # Unfortunately, not all the pull request information we + # need is available in the workflow_run payload. We need + # to fetch it from the API. + - name: Get Pull Request Information + if: github.event.workflow_run.event == 'pull_request' + uses: octokit/request-action@v2.0.2 + id: pull_request + with: + route: GET /repos/{owner}/{repo}/pulls + owner: ${{ github.event.repository.owner.login }} + repo: ${{ github.event.repository.name }} + state: open + head: ${{format( + '{0}:{1}', + github.event.workflow_run.head_repository.owner.login, + github.event.workflow_run.head_branch + )}} + sort: updated + direction: desc + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + # Send an informational status embed to Discord instead of the + # standard embeds that Discord sends. This embed will contain + # more information and we can fine tune when we actually want + # to send an embed. + - name: GitHub Actions Status Embed for Discord + uses: SebastiaanZ/github-status-embed-for-discord@v0.2.1 + with: + # Our GitHub Actions webhook + webhook_id: '784184528997842985' + webhook_token: ${{ secrets.GHA_WEBHOOK_TOKEN }} + + # Workflow information + workflow_name: ${{ github.event.workflow_run.name }} + run_id: ${{ github.event.workflow_run.id }} + run_number: ${{ github.event.workflow_run.run_number }} + status: ${{ github.event.workflow_run.conclusion }} + actor: ${{ github.actor }} + repository: ${{ github.repository }} + ref: ${{ github.ref }} + sha: ${{ github.event.workflow_run.head_sha }} + + pull_request_payload: ${{ steps.pull_request.outputs.data }} -- cgit v1.2.3 From eacaf581ea5e0953c48f14c7f9801a0d661e6c89 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Thu, 10 Dec 2020 13:39:17 +0100 Subject: Use Build Artifact to communicate PR information A workflow run with a `workflow_run` payload does not contain the necessary information to build a PR embed. As the old method of using the API to fetch the relevant information turned out to be fragile (read note 1 below) and the original Lint workflow already contains the `pull_request` payload, we now store it as a build artifact. The embed workflow then fetches the artifact and parses it to get the relevant information out of it. --- Note 1: Unfortunately, filtering Pull Requests using the "head" parameter of the ``/repos/{owner}/{repo}/pulls` endpoint does not work if the PR belongs to a fork with a different name than the base repository: the API request will just return an empty array. I've contacted GH to ask if this was intended or if it's a glitch, but, for now, it's not a route that's easily available. --- .github/workflows/lint.yaml | 22 ++++++++++++++++++++++ .github/workflows/status_embed.yaml | 37 +++++++++++++++++++------------------ 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml index 063f406c..c0822e7f 100644 --- a/.github/workflows/lint.yaml +++ b/.github/workflows/lint.yaml @@ -91,3 +91,25 @@ jobs: - name: Run flake8 run: "flake8 \ --format='::error file=%(path)s,line=%(row)d,col=%(col)d::[flake8] %(code)s: %(text)s'" + + # Prepare the Pull Request Payload artifact. If this fails, we + # we fail silently using the `continue-on-error` option. It's + # nice if this succeeds, but if it fails for any reason, it + # does not mean that our lint checks failed. + - name: Prepare Pull Request Payload artifact + id: prepare-artifact + if: always() && github.event_name == 'pull_request' + continue-on-error: true + run: cat $GITHUB_EVENT_PATH | jq '.pull_request' > pull_request_payload.json + + # This only makes sense if the previous step succeeded. To + # get the original outcome of the previous step before the + # `continue-on-error` conclusion is applied, we use the + # `.outcome` value. This step also fails silently. + - name: Upload a Build Artifact + if: steps.prepare-artifact.outcome == 'success' + continue-on-error: true + uses: actions/upload-artifact@v2 + with: + name: pull-request-payload + path: pull_request_payload.json diff --git a/.github/workflows/status_embed.yaml b/.github/workflows/status_embed.yaml index 1d175fb9..c8502a19 100644 --- a/.github/workflows/status_embed.yaml +++ b/.github/workflows/status_embed.yaml @@ -25,25 +25,23 @@ jobs: runs-on: ubuntu-latest steps: - # Unfortunately, not all the pull request information we - # need is available in the workflow_run payload. We need - # to fetch it from the API. + # A workflow_run event does not contain all the information + # we need for a PR embed. That's why we upload an artifact + # with that information in the Lint workflow. - name: Get Pull Request Information + id: pr_info if: github.event.workflow_run.event == 'pull_request' - uses: octokit/request-action@v2.0.2 - id: pull_request - with: - route: GET /repos/{owner}/{repo}/pulls - owner: ${{ github.event.repository.owner.login }} - repo: ${{ github.event.repository.name }} - state: open - head: ${{format( - '{0}:{1}', - github.event.workflow_run.head_repository.owner.login, - github.event.workflow_run.head_branch - )}} - sort: updated - direction: desc + run: | + curl -s -H "Authorization: token $GITHUB_TOKEN" ${{ github.event.workflow_run.artifacts_url }} > artifacts.json + DOWNLOAD_URL=$(cat artifacts.json | jq -r '.artifacts[] | select(.name == "pull-request-payload") | .archive_download_url') + [ -z "$DOWNLOAD_URL" ] && exit 1 + wget --quiet --header="Authorization: token $GITHUB_TOKEN" -O pull_request_payload.zip $DOWNLOAD_URL || exit 2 + unzip -p pull_request_payload.zip > pull_request_payload.json + [ -s pull_request_payload.json ] || exit 3 + echo "::set-output name=pr_author_login::$(jq -r '.user.login // empty' pull_request_payload.json)" + echo "::set-output name=pr_number::$(jq -r '.number // empty' pull_request_payload.json)" + echo "::set-output name=pr_title::$(jq -r '.title // empty' pull_request_payload.json)" + echo "::set-output name=pr_source::$(jq -r '.head.label // empty' pull_request_payload.json)" env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} @@ -68,4 +66,7 @@ jobs: ref: ${{ github.ref }} sha: ${{ github.event.workflow_run.head_sha }} - pull_request_payload: ${{ steps.pull_request.outputs.data }} + pr_author_login: ${{ steps.pr_info.outputs.pr_author_login }} + pr_number: ${{ steps.pr_info.outputs.pr_number }} + pr_title: ${{ steps.pr_info.outputs.pr_title }} + pr_source: ${{ steps.pr_info.outputs.pr_source }} -- cgit v1.2.3 From b92e802d2c1768ecfb3d8c9dab49036d9a9fe528 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com> Date: Thu, 10 Dec 2020 14:59:10 +0100 Subject: Skip status embed for skipped Build workflow With the new `workflow_run` setup, we need to make sure that we don't try to send an embed for a skipped workflow. This would be noisy and does not add a lot of useful information. --- .github/workflows/status_embed.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/status_embed.yaml b/.github/workflows/status_embed.yaml index c8502a19..28caa8c2 100644 --- a/.github/workflows/status_embed.yaml +++ b/.github/workflows/status_embed.yaml @@ -18,9 +18,10 @@ jobs: # - Always run for non-success workflows, as they # terminate the workflow sequence. if: >- - github.event.workflow_run.name == 'Build' || + (github.event.workflow_run.name == 'Build' && github.event.workflow_run.conclusion != 'skipped') || github.event.workflow_run.event == 'pull_request' || - github.event.workflow_run.conclusion != 'success' + github.event.workflow_run.conclusion == 'failure' || + github.event.workflow_run.conclusion == 'cancelled' name: Send Status Embed to Discord runs-on: ubuntu-latest -- cgit v1.2.3 From 40fce53d6c01f0e753d70cb4b265d88f1107e104 Mon Sep 17 00:00:00 2001 From: Rohan Date: Wed, 9 Dec 2020 23:13:39 +0530 Subject: Check if error.original is an instance of OSError. Also, remove error handler for get_command and video_command. --- bot/exts/evergreen/snakes/_snakes_cog.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/bot/exts/evergreen/snakes/_snakes_cog.py b/bot/exts/evergreen/snakes/_snakes_cog.py index 2e88c146..4fa4dcd1 100644 --- a/bot/exts/evergreen/snakes/_snakes_cog.py +++ b/bot/exts/evergreen/snakes/_snakes_cog.py @@ -1126,16 +1126,15 @@ class Snakes(Cog): # endregion # region: Error handlers - @get_command.error @card_command.error - @video_command.error async def command_error(self, ctx: Context, error: CommandError) -> None: """Local error handler for the Snake Cog.""" - if isinstance(error, OSError): + original_error = getattr(error, "original", None) + if isinstance(original_error, OSError): error.handled = True embed = Embed() embed.colour = Colour.red() - log.error(f"snake_card encountered an OSError: {error} ({error.original})") + log.error(f"snake_card encountered an OSError: {error} ({original_error})") embed.description = "Could not generate the snake card! Please try again." embed.title = random.choice(ERROR_REPLIES) await ctx.send(embed=embed) -- cgit v1.2.3 From b691aedb81a7cc35067f4ddfda0127d7ca86af6a Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com> Date: Fri, 11 Dec 2020 10:21:44 +0100 Subject: Make sure PR artifacts are always uploaded We need to upload PR artifacts regardless of our lint job failing. As GitHub Actions uses an implicit "success" status function if you don't specify one of your own, we need to include the "always" function in our condition. --- .github/workflows/lint.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml index c0822e7f..a5f45255 100644 --- a/.github/workflows/lint.yaml +++ b/.github/workflows/lint.yaml @@ -107,7 +107,7 @@ jobs: # `continue-on-error` conclusion is applied, we use the # `.outcome` value. This step also fails silently. - name: Upload a Build Artifact - if: steps.prepare-artifact.outcome == 'success' + if: always() && steps.prepare-artifact.outcome == 'success' continue-on-error: true uses: actions/upload-artifact@v2 with: -- cgit v1.2.3 From d884c21e8ae2bd82370b11621230d7172cb098a3 Mon Sep 17 00:00:00 2001 From: janine9vn Date: Wed, 2 Dec 2020 09:28:32 -0500 Subject: Disallow .aoc commands in primary aoc channel Commands like `.aoc leaderboard` and `.aoc stats` proved to be spammy in the main advent of code channel. An aoc_commands channel has been added for aoc commands and this update prohibits aoc commands from being used in the primary aoc channel and adds the comands channel to the whitelist. This also specifically allows the less spammier commands: join, subscribe, unsubscribe, and countdown in the primary channel to foster discussion though. --- bot/constants.py | 1 + bot/exts/christmas/advent_of_code/_cog.py | 10 +++++----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/bot/constants.py b/bot/constants.py index 9e6db7a6..5dc42462 100644 --- a/bot/constants.py +++ b/bot/constants.py @@ -95,6 +95,7 @@ class Branding: class Channels(NamedTuple): admins = 365960823622991872 advent_of_code = int(environ.get("AOC_CHANNEL_ID", 782715290437943306)) + advent_of_code_commands = int(environ.get("AOC_COMMANDS_CHANNEL_ID", 783503267849437205)) announcements = int(environ.get("CHANNEL_ANNOUNCEMENTS", 354619224620138496)) big_brother_logs = 468507907357409333 bot = 267659945086812160 diff --git a/bot/exts/christmas/advent_of_code/_cog.py b/bot/exts/christmas/advent_of_code/_cog.py index 0bcd9f42..fad13f23 100644 --- a/bot/exts/christmas/advent_of_code/_cog.py +++ b/bot/exts/christmas/advent_of_code/_cog.py @@ -21,7 +21,7 @@ AOC_REQUEST_HEADER = {"user-agent": "PythonDiscord AoC Event Bot"} COUNTDOWN_STEP = 60 * 5 -AOC_WHITELIST = WHITELISTED_CHANNELS + (Channels.advent_of_code,) +AOC_WHITELIST = WHITELISTED_CHANNELS + (Channels.advent_of_code_commands,) async def countdown_status(bot: commands.Bot) -> None: @@ -139,7 +139,7 @@ class AdventOfCode(commands.Cog): aliases=("sub", "notifications", "notify", "notifs"), brief="Notifications for new days" ) - @override_in_channel(AOC_WHITELIST) + @override_in_channel(AOC_WHITELIST + (Channels.advent_of_code)) async def aoc_subscribe(self, ctx: commands.Context) -> None: """Assign the role for notifications about new days being ready.""" current_year = datetime.now().year @@ -160,7 +160,7 @@ class AdventOfCode(commands.Cog): @in_month(Month.DECEMBER) @adventofcode_group.command(name="unsubscribe", aliases=("unsub",), brief="Notifications for new days") - @override_in_channel(AOC_WHITELIST) + @override_in_channel(AOC_WHITELIST + Channels.advent_of_code) async def aoc_unsubscribe(self, ctx: commands.Context) -> None: """Remove the role for notifications about new days being ready.""" role = ctx.guild.get_role(AocConfig.role_id) @@ -172,7 +172,7 @@ class AdventOfCode(commands.Cog): await ctx.send("Hey, you don't even get any notifications about new Advent of Code tasks currently anyway.") @adventofcode_group.command(name="countdown", aliases=("count", "c"), brief="Return time left until next day") - @override_in_channel(AOC_WHITELIST) + @override_in_channel(AOC_WHITELIST + (Channels.advent_of_code)) async def aoc_countdown(self, ctx: commands.Context) -> None: """Return time left until next day.""" if not _helpers.is_in_advent(): @@ -207,7 +207,7 @@ class AdventOfCode(commands.Cog): await ctx.send("", embed=self.cached_about_aoc) @adventofcode_group.command(name="join", aliases=("j",), brief="Learn how to join the leaderboard (via DM)") - @override_in_channel(AOC_WHITELIST) + @override_in_channel(AOC_WHITELIST + (Channels.advent_of_code)) async def join_leaderboard(self, ctx: commands.Context) -> None: """DM the user the information for joining the Python Discord leaderboard.""" current_year = datetime.now().year -- cgit v1.2.3 From d222ee24ec4d90c4eacbe50348fb1e9b6f1a0fed Mon Sep 17 00:00:00 2001 From: janine9vn Date: Wed, 2 Dec 2020 11:15:03 -0500 Subject: Add cog-level error handler for Incorrect Channel If any of the "spammier" commands (stats, leaderboard) are used within the primary advent of code channel, rather than a non-specific embed we instead reply with the channel they should be using. This also adds a "AOC_WHITELIST_PLUS" constant that makes it easier to adjust what channels the non-spammier aoc commands can be used in. --- bot/exts/christmas/advent_of_code/_cog.py | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/bot/exts/christmas/advent_of_code/_cog.py b/bot/exts/christmas/advent_of_code/_cog.py index fad13f23..30030e70 100644 --- a/bot/exts/christmas/advent_of_code/_cog.py +++ b/bot/exts/christmas/advent_of_code/_cog.py @@ -13,7 +13,7 @@ from bot.constants import ( AdventOfCode as AocConfig, Channels, Colours, Emojis, Month, Roles, WHITELISTED_CHANNELS, ) from bot.exts.christmas.advent_of_code import _helpers -from bot.utils.decorators import in_month, override_in_channel, with_role +from bot.utils.decorators import InChannelCheckFailure, in_month, override_in_channel, with_role log = logging.getLogger(__name__) @@ -23,6 +23,10 @@ COUNTDOWN_STEP = 60 * 5 AOC_WHITELIST = WHITELISTED_CHANNELS + (Channels.advent_of_code_commands,) +# Some commands can be run in the regular advent of code channel +# They aren't spammy and foster discussion +AOC_WHITELIST_PLUS = AOC_WHITELIST + (Channels.advent_of_code,) + async def countdown_status(bot: commands.Bot) -> None: """Set the playing status of the bot to the minutes & hours left until the next day's challenge.""" @@ -128,7 +132,7 @@ class AdventOfCode(commands.Cog): self.status_task = self.bot.loop.create_task(status_coro) @commands.group(name="adventofcode", aliases=("aoc",)) - @override_in_channel(AOC_WHITELIST) + @override_in_channel(AOC_WHITELIST_PLUS) async def adventofcode_group(self, ctx: commands.Context) -> None: """All of the Advent of Code commands.""" if not ctx.invoked_subcommand: @@ -139,7 +143,7 @@ class AdventOfCode(commands.Cog): aliases=("sub", "notifications", "notify", "notifs"), brief="Notifications for new days" ) - @override_in_channel(AOC_WHITELIST + (Channels.advent_of_code)) + @override_in_channel(AOC_WHITELIST_PLUS) async def aoc_subscribe(self, ctx: commands.Context) -> None: """Assign the role for notifications about new days being ready.""" current_year = datetime.now().year @@ -160,7 +164,7 @@ class AdventOfCode(commands.Cog): @in_month(Month.DECEMBER) @adventofcode_group.command(name="unsubscribe", aliases=("unsub",), brief="Notifications for new days") - @override_in_channel(AOC_WHITELIST + Channels.advent_of_code) + @override_in_channel(AOC_WHITELIST_PLUS) async def aoc_unsubscribe(self, ctx: commands.Context) -> None: """Remove the role for notifications about new days being ready.""" role = ctx.guild.get_role(AocConfig.role_id) @@ -172,7 +176,7 @@ class AdventOfCode(commands.Cog): await ctx.send("Hey, you don't even get any notifications about new Advent of Code tasks currently anyway.") @adventofcode_group.command(name="countdown", aliases=("count", "c"), brief="Return time left until next day") - @override_in_channel(AOC_WHITELIST + (Channels.advent_of_code)) + @override_in_channel(AOC_WHITELIST_PLUS) async def aoc_countdown(self, ctx: commands.Context) -> None: """Return time left until next day.""" if not _helpers.is_in_advent(): @@ -207,7 +211,7 @@ class AdventOfCode(commands.Cog): await ctx.send("", embed=self.cached_about_aoc) @adventofcode_group.command(name="join", aliases=("j",), brief="Learn how to join the leaderboard (via DM)") - @override_in_channel(AOC_WHITELIST + (Channels.advent_of_code)) + @override_in_channel(AOC_WHITELIST_PLUS) async def join_leaderboard(self, ctx: commands.Context) -> None: """DM the user the information for joining the Python Discord leaderboard.""" current_year = datetime.now().year @@ -366,3 +370,11 @@ class AdventOfCode(commands.Cog): about_embed.set_footer(text="Last Updated") return about_embed + + async def cog_command_error(self, ctx: commands.Context, error: Exception) -> None: + """Custom error handler if an advent of code command was posted in the wrong channel.""" + if isinstance(error, InChannelCheckFailure): + await ctx.send(f":x: Please use <#{Channels.advent_of_code_commands}> for aoc commands instead, please.") + ctx.command.on_error = True + else: + raise error -- cgit v1.2.3 From 15a511d5895c5c1f9ec68b968fa4e0ff55a416a3 Mon Sep 17 00:00:00 2001 From: janine9vn Date: Wed, 2 Dec 2020 11:21:27 -0500 Subject: Change custom error handler to match new style I'm a bit ahead of the game and changing the error handler to match the new style that Iceman will PR shortly. --- bot/exts/christmas/advent_of_code/_cog.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/exts/christmas/advent_of_code/_cog.py b/bot/exts/christmas/advent_of_code/_cog.py index 30030e70..0ad718b9 100644 --- a/bot/exts/christmas/advent_of_code/_cog.py +++ b/bot/exts/christmas/advent_of_code/_cog.py @@ -375,6 +375,6 @@ class AdventOfCode(commands.Cog): """Custom error handler if an advent of code command was posted in the wrong channel.""" if isinstance(error, InChannelCheckFailure): await ctx.send(f":x: Please use <#{Channels.advent_of_code_commands}> for aoc commands instead, please.") - ctx.command.on_error = True + error.handled = True else: raise error -- cgit v1.2.3 From b6fa6385c82e216d9ef5d2cefe1dd1efc470008f Mon Sep 17 00:00:00 2001 From: janine9vn Date: Fri, 11 Dec 2020 15:03:57 -0500 Subject: Remove re-raising the error Per Mark's comment, re-raising the error isn't necessary. --- bot/exts/christmas/advent_of_code/_cog.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/bot/exts/christmas/advent_of_code/_cog.py b/bot/exts/christmas/advent_of_code/_cog.py index 0ad718b9..b6462ab2 100644 --- a/bot/exts/christmas/advent_of_code/_cog.py +++ b/bot/exts/christmas/advent_of_code/_cog.py @@ -376,5 +376,3 @@ class AdventOfCode(commands.Cog): if isinstance(error, InChannelCheckFailure): await ctx.send(f":x: Please use <#{Channels.advent_of_code_commands}> for aoc commands instead, please.") error.handled = True - else: - raise error -- cgit v1.2.3 From f493699a4a076bebcd69404be57c3a32da5a22a1 Mon Sep 17 00:00:00 2001 From: janine9vn Date: Fri, 11 Dec 2020 17:11:41 -0500 Subject: Remove extra please Please -= 1 --- bot/exts/christmas/advent_of_code/_cog.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/exts/christmas/advent_of_code/_cog.py b/bot/exts/christmas/advent_of_code/_cog.py index b6462ab2..90d92fc1 100644 --- a/bot/exts/christmas/advent_of_code/_cog.py +++ b/bot/exts/christmas/advent_of_code/_cog.py @@ -374,5 +374,5 @@ class AdventOfCode(commands.Cog): async def cog_command_error(self, ctx: commands.Context, error: Exception) -> None: """Custom error handler if an advent of code command was posted in the wrong channel.""" if isinstance(error, InChannelCheckFailure): - await ctx.send(f":x: Please use <#{Channels.advent_of_code_commands}> for aoc commands instead, please.") + await ctx.send(f":x: Please use <#{Channels.advent_of_code_commands}> for aoc commands instead.") error.handled = True -- cgit v1.2.3 From 0ca1c0b55ea5d07694dc53d4f4856cbcddb35a8f Mon Sep 17 00:00:00 2001 From: janine9vn Date: Fri, 11 Dec 2020 19:30:23 -0500 Subject: Change Default AoC Commands Channel Changes the default value of the advent_of_code_commands constant to be the same channel ID as sir-lancebot-commands. If no AoC commands channel is set in the .env file, it'll re-direct people to sir-lancebot-commands instead. --- bot/constants.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/constants.py b/bot/constants.py index 5dc42462..a58801f7 100644 --- a/bot/constants.py +++ b/bot/constants.py @@ -95,7 +95,7 @@ class Branding: class Channels(NamedTuple): admins = 365960823622991872 advent_of_code = int(environ.get("AOC_CHANNEL_ID", 782715290437943306)) - advent_of_code_commands = int(environ.get("AOC_COMMANDS_CHANNEL_ID", 783503267849437205)) + advent_of_code_commands = int(environ.get("AOC_COMMANDS_CHANNEL_ID", 607247579608121354)) announcements = int(environ.get("CHANNEL_ANNOUNCEMENTS", 354619224620138496)) big_brother_logs = 468507907357409333 bot = 267659945086812160 -- cgit v1.2.3 From 8f119f66ec52b1ad55f2078289d35632d0c8127a Mon Sep 17 00:00:00 2001 From: janine9vn Date: Fri, 11 Dec 2020 19:43:43 -0500 Subject: Change AOC_WHITELIST names for clarity AOC_WHITELIST was changed to AOC_WHITELIST_RESTRICTED because it is clearer that commands with this parameter in the `@override_in_channel()` decorator will be restricted to the aoc commands channel and not be allowed in the main aoc channel. In the same vein, AOC_WHITELIST_PLUS was changed to AOC_WHITELIST. --- bot/exts/christmas/advent_of_code/_cog.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/bot/exts/christmas/advent_of_code/_cog.py b/bot/exts/christmas/advent_of_code/_cog.py index 90d92fc1..0968dd26 100644 --- a/bot/exts/christmas/advent_of_code/_cog.py +++ b/bot/exts/christmas/advent_of_code/_cog.py @@ -21,11 +21,11 @@ AOC_REQUEST_HEADER = {"user-agent": "PythonDiscord AoC Event Bot"} COUNTDOWN_STEP = 60 * 5 -AOC_WHITELIST = WHITELISTED_CHANNELS + (Channels.advent_of_code_commands,) +AOC_WHITELIST_RESTRICTED = WHITELISTED_CHANNELS + (Channels.advent_of_code_commands,) # Some commands can be run in the regular advent of code channel # They aren't spammy and foster discussion -AOC_WHITELIST_PLUS = AOC_WHITELIST + (Channels.advent_of_code,) +AOC_WHITELIST = AOC_WHITELIST_RESTRICTED + (Channels.advent_of_code,) async def countdown_status(bot: commands.Bot) -> None: @@ -132,7 +132,7 @@ class AdventOfCode(commands.Cog): self.status_task = self.bot.loop.create_task(status_coro) @commands.group(name="adventofcode", aliases=("aoc",)) - @override_in_channel(AOC_WHITELIST_PLUS) + @override_in_channel(AOC_WHITELIST) async def adventofcode_group(self, ctx: commands.Context) -> None: """All of the Advent of Code commands.""" if not ctx.invoked_subcommand: @@ -143,7 +143,7 @@ class AdventOfCode(commands.Cog): aliases=("sub", "notifications", "notify", "notifs"), brief="Notifications for new days" ) - @override_in_channel(AOC_WHITELIST_PLUS) + @override_in_channel(AOC_WHITELIST) async def aoc_subscribe(self, ctx: commands.Context) -> None: """Assign the role for notifications about new days being ready.""" current_year = datetime.now().year @@ -164,7 +164,7 @@ class AdventOfCode(commands.Cog): @in_month(Month.DECEMBER) @adventofcode_group.command(name="unsubscribe", aliases=("unsub",), brief="Notifications for new days") - @override_in_channel(AOC_WHITELIST_PLUS) + @override_in_channel(AOC_WHITELIST) async def aoc_unsubscribe(self, ctx: commands.Context) -> None: """Remove the role for notifications about new days being ready.""" role = ctx.guild.get_role(AocConfig.role_id) @@ -176,7 +176,7 @@ class AdventOfCode(commands.Cog): await ctx.send("Hey, you don't even get any notifications about new Advent of Code tasks currently anyway.") @adventofcode_group.command(name="countdown", aliases=("count", "c"), brief="Return time left until next day") - @override_in_channel(AOC_WHITELIST_PLUS) + @override_in_channel(AOC_WHITELIST) async def aoc_countdown(self, ctx: commands.Context) -> None: """Return time left until next day.""" if not _helpers.is_in_advent(): @@ -211,7 +211,7 @@ class AdventOfCode(commands.Cog): await ctx.send("", embed=self.cached_about_aoc) @adventofcode_group.command(name="join", aliases=("j",), brief="Learn how to join the leaderboard (via DM)") - @override_in_channel(AOC_WHITELIST_PLUS) + @override_in_channel(AOC_WHITELIST) async def join_leaderboard(self, ctx: commands.Context) -> None: """DM the user the information for joining the Python Discord leaderboard.""" current_year = datetime.now().year @@ -260,7 +260,7 @@ class AdventOfCode(commands.Cog): aliases=("board", "lb"), brief="Get a snapshot of the PyDis private AoC leaderboard", ) - @override_in_channel(AOC_WHITELIST) + @override_in_channel(AOC_WHITELIST_RESTRICTED) async def aoc_leaderboard(self, ctx: commands.Context) -> None: """Get the current top scorers of the Python Discord Leaderboard.""" async with ctx.typing(): @@ -285,7 +285,7 @@ class AdventOfCode(commands.Cog): aliases=("globalboard", "gb"), brief="Get a link to the global leaderboard", ) - @override_in_channel(AOC_WHITELIST) + @override_in_channel(AOC_WHITELIST_RESTRICTED) async def aoc_global_leaderboard(self, ctx: commands.Context) -> None: """Get a link to the global Advent of Code leaderboard.""" url = self.global_leaderboard_url @@ -301,7 +301,7 @@ class AdventOfCode(commands.Cog): aliases=("dailystats", "ds"), brief="Get daily statistics for the Python Discord leaderboard" ) - @override_in_channel(AOC_WHITELIST) + @override_in_channel(AOC_WHITELIST_RESTRICTED) async def private_leaderboard_daily_stats(self, ctx: commands.Context) -> None: """Send an embed with daily completion statistics for the Python Discord leaderboard.""" try: -- cgit v1.2.3 From 0a91d49969600fa2d5f5b9e429ff693f3b94da72 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Sun, 13 Dec 2020 10:40:56 +0100 Subject: Add callback to log errors in AoC background tasks Currently, our Advent of Code background tasks fail without logging errors or printing error messages. This makes it difficult to debug the errors and means that they may fail silently. While we should ideally find the root cause that hides such errors, I've added a done_callback function in the meantime to help us debug the current issues with the Advent of Code Notification Task. --- bot/exts/christmas/advent_of_code/_cog.py | 6 ++++++ bot/exts/christmas/advent_of_code/_helpers.py | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/bot/exts/christmas/advent_of_code/_cog.py b/bot/exts/christmas/advent_of_code/_cog.py index 0968dd26..0671203e 100644 --- a/bot/exts/christmas/advent_of_code/_cog.py +++ b/bot/exts/christmas/advent_of_code/_cog.py @@ -30,6 +30,7 @@ AOC_WHITELIST = AOC_WHITELIST_RESTRICTED + (Channels.advent_of_code,) async def countdown_status(bot: commands.Bot) -> None: """Set the playing status of the bot to the minutes & hours left until the next day's challenge.""" + log.info("Started `AoC Status Countdown` task") while _helpers.is_in_advent(): _, time_left = _helpers.time_left_to_aoc_midnight() @@ -62,6 +63,7 @@ async def day_countdown(bot: commands.Bot) -> None: Once we have calculated this we should then sleep that number and when the time is reached, ping the Advent of Code role notifying them that the new challenge is ready. """ + log.info("Started `Daily AoC Notification` task") while _helpers.is_in_advent(): tomorrow, time_left = _helpers.time_left_to_aoc_midnight() @@ -127,9 +129,13 @@ class AdventOfCode(commands.Cog): countdown_coro = day_countdown(self.bot) self.countdown_task = self.bot.loop.create_task(countdown_coro) + self.countdown_task.set_name("Daily AoC Notification") + self.countdown_task.add_done_callback(_helpers.background_task_callback) status_coro = countdown_status(self.bot) self.status_task = self.bot.loop.create_task(status_coro) + self.status_task.set_name("AoC Status Countdown") + self.status_task.add_done_callback(_helpers.background_task_callback) @commands.group(name="adventofcode", aliases=("aoc",)) @override_in_channel(AOC_WHITELIST) diff --git a/bot/exts/christmas/advent_of_code/_helpers.py b/bot/exts/christmas/advent_of_code/_helpers.py index d883c09f..da139e40 100644 --- a/bot/exts/christmas/advent_of_code/_helpers.py +++ b/bot/exts/christmas/advent_of_code/_helpers.py @@ -1,3 +1,4 @@ +import asyncio import collections import datetime import json @@ -407,3 +408,13 @@ def time_left_to_aoc_midnight() -> Tuple[datetime.datetime, datetime.timedelta]: # Calculate the timedelta between the current time and midnight return tomorrow, tomorrow - datetime.datetime.now(EST) + + +def background_task_callback(task: asyncio.Task) -> None: + """Check if the finished background task failed to make sure we log errors.""" + if task.cancelled(): + log.info(f"Background task `{task.get_name()}` was cancelled.") + elif exception := task.exception(): + log.error(f"Background task `{task.get_name()}` failed:", exc_info=exception) + else: + log.info(f"Background task `{task.get_name()}` exited normally.") -- cgit v1.2.3