From 0c1740fe2aa32b7fcc7f143c740cf6bc6c42afe5 Mon Sep 17 00:00:00 2001 From: Dana Date: Mon, 14 Aug 2023 11:37:29 +0300 Subject: [PATCH 1/7] show patch coverage only for changed files, and don't show indirect changed files --- .../notifiers/mixins/message/helpers.py | 47 +++++++++++++++++++ .../notifiers/mixins/message/sections.py | 17 +++---- .../notifiers/tests/unit/test_comment.py | 8 ++-- 3 files changed, 59 insertions(+), 13 deletions(-) diff --git a/services/notification/notifiers/mixins/message/helpers.py b/services/notification/notifiers/mixins/message/helpers.py index d72edbb2b..95ffd6232 100644 --- a/services/notification/notifiers/mixins/message/helpers.py +++ b/services/notification/notifiers/mixins/message/helpers.py @@ -116,6 +116,53 @@ def make_metrics(before, after, relative, show_complexity, yaml): return "".join(("|", coverage, complexity, icon)) +def make_patch_only_metrics(before, after, relative, show_complexity, yaml): + if after is None: + # e.g. missing flags + coverage = " `?` |" + + elif after is False: + # e.g. file deleted + coverage = " |" + + else: + layout = " `{relative}` |" + coverage = layout.format( + relative=format_number_to_str( + yaml, relative.coverage if relative else 0, style="{0}%", if_null="\xF8" + ), + ) + return "".join(("|", coverage)) + + +def get_metrics_method(hide_project_coverage): + if hide_project_coverage: + metrics = make_patch_only_metrics + else: + metrics = make_metrics + return metrics + + +def get_table_header(hide_project_coverage, show_complexity): + if not hide_project_coverage: + table_header = ( + "| Coverage \u0394 |" + + (" Complexity \u0394 |" if show_complexity else "") + + " |" + ) + else: + table_header = "| Coverage |" + + return table_header + + +def get_table_layout(hide_project_coverage, show_complexity): + if hide_project_coverage: + return "|---|---|" + else: + return "|---|---|---|" + ("---|" if show_complexity else "") + + def format_number_to_str( yml, value, if_zero=None, if_null=None, plus=False, style="{0}" ) -> str: diff --git a/services/notification/notifiers/mixins/message/sections.py b/services/notification/notifiers/mixins/message/sections.py index ead1cc00c..1d5efd26a 100644 --- a/services/notification/notifiers/mixins/message/sections.py +++ b/services/notification/notifiers/mixins/message/sections.py @@ -20,6 +20,9 @@ diff_to_string, ellipsis, escape_markdown, + get_metrics_method, + get_table_header, + get_table_layout, make_metrics, sort_by_importance, ) @@ -464,11 +467,13 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non head_report = comparison.head.report if base_report is None: base_report = Report() + hide_project_coverage = self.settings.get("hide_project_coverage", False) + metrics = get_metrics_method(hide_project_coverage) files_in_diff = [ ( _diff["type"], path, - make_metrics( + metrics( get_totals_from_file_in_reports(base_report, path) or False, get_totals_from_file_in_reports(head_report, path) or False, _diff["totals"], @@ -487,12 +492,8 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non c.path for c in changes or [] ) if files_in_diff: - table_header = ( - "| Coverage \u0394 |" - + (" Complexity \u0394 |" if self.show_complexity else "") - + " |" - ) - table_layout = "|---|---|---|" + ("---|" if self.show_complexity else "") + table_header = get_table_header(hide_project_coverage, self.show_complexity) + table_layout = get_table_layout(hide_project_coverage, self.show_complexity) # add table headers yield ( "| [Files Changed]({0}?src=pr&el=tree) {1}".format( @@ -539,7 +540,7 @@ def tree_cell(typ, path, metrics, _=None): ) ) - if changes: + if changes and not hide_project_coverage: len_changes_not_in_diff = len(all_files or []) - len(files_in_diff or []) if files_in_diff and len_changes_not_in_diff > 0: yield ("") diff --git a/services/notification/notifiers/tests/unit/test_comment.py b/services/notification/notifiers/tests/unit/test_comment.py index 995fc9017..f4038dd8d 100644 --- a/services/notification/notifiers/tests/unit/test_comment.py +++ b/services/notification/notifiers/tests/unit/test_comment.py @@ -4163,11 +4163,9 @@ async def test_build_message_no_project_coverage( f"## [Codecov](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=h1) Report", f"Patch coverage is **`66.67%`** of modified lines.", f"", - f"| [Files Changed](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=tree) | Coverage Δ | Complexity Δ | |", - f"|---|---|---|---|", - f"| [file\\_1.go](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=tree#diff-ZmlsZV8xLmdv) | `62.50% <66.67%> (+12.50%)` | `10.00 <0.00> (-1.00)` | :arrow_up: |", - f"", - f"... and [1 file with indirect coverage changes](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/indirect-changes?src=pr&el=tree-more)", + f"| [Files Changed](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=tree) | Coverage |", + f"|---|---|", + f"| [file\\_1.go](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=tree#diff-ZmlsZV8xLmdv) | `66.67%` |", f"", f"", f"[:umbrella: View full report in Codecov by Sentry](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=continue). ", From b3142a846ec697f7d40aa096983734d179d78115 Mon Sep 17 00:00:00 2001 From: Scott Nelson Date: Mon, 14 Aug 2023 10:31:33 -0400 Subject: [PATCH 2/7] feat: Link flag and component names in PR comment back to dashboard pages --- .../notifiers/mixins/message/sections.py | 19 ++- .../tests/integration/test_comment.py | 34 ++--- .../notifiers/tests/unit/test_comment.py | 126 +++++++++--------- tasks/tests/integration/test_notify_task.py | 4 +- 4 files changed, 97 insertions(+), 86 deletions(-) diff --git a/services/notification/notifiers/mixins/message/sections.py b/services/notification/notifiers/mixins/message/sections.py index ead1cc00c..0bd850969 100644 --- a/services/notification/notifiers/mixins/message/sections.py +++ b/services/notification/notifiers/mixins/message/sections.py @@ -628,7 +628,10 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non + ("---|" if has_carriedforward_flags else "") ) - yield ("| Flag " + table_header) + yield ( + "| [Flag]({href}/flags?src=pr&el=flags) ".format(href=links["pull"]) + + table_header + ) yield (table_layout) for flag in sorted(flags, key=lambda f: f["name"]): carriedforward, carriedforward_from = ( @@ -660,7 +663,10 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non yield ( "| {name} {metrics}{cf}".format( - name=flag["name"], + name="[{flag_name}]({href}/flags?src=pr&el=flag)".format( + flag_name=flag["name"], + href=links["pull"], + ), metrics=make_metrics( flag["before"], flag["after"], @@ -721,13 +727,18 @@ async def do_write_section( ) # Table header and layout - yield "| Components | Coverage \u0394 | |" + yield "| [Components]({href}/components?src=pr&el=components) | Coverage \u0394 | |".format( + href=links["pull"], + ) yield "|---|---|---|" # The interesting part for component_data in component_data_to_show: yield ( "| {name} {metrics}".format( - name=component_data["name"], + name="[{component_name}]({href}/components?src=pr&el=component)".format( + component_name=component_data["name"], + href=links["pull"], + ), metrics=make_metrics( component_data["before"], component_data["after"], diff --git a/services/notification/notifiers/tests/integration/test_comment.py b/services/notification/notifiers/tests/integration/test_comment.py index 91a38baa2..db265a0a9 100644 --- a/services/notification/notifiers/tests/integration/test_comment.py +++ b/services/notification/notifiers/tests/integration/test_comment.py @@ -364,10 +364,10 @@ async def test_notify(self, sample_comparison, codecov_vcr, mock_configuration): "- Partials 0 1 +1 ", "```", "", - "| Flag | Coverage Δ | Complexity Δ | |", + "| [Flag](https://app.codecov.io/gh/ThiagoCodecov/example-python/pull/15/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", "|---|---|---|---|", - "| integration | `?` | `?` | |", - "| unit | `100.00% <ø> (?)` | `0.00 <ø> (?)` | |", + "| [integration](https://app.codecov.io/gh/ThiagoCodecov/example-python/pull/15/flags?src=pr&el=flag) | `?` | `?` | |", + "| [unit](https://app.codecov.io/gh/ThiagoCodecov/example-python/pull/15/flags?src=pr&el=flag) | `100.00% <ø> (?)` | `0.00 <ø> (?)` | |", "", "Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", "", @@ -431,7 +431,7 @@ async def test_notify_upload_limited( ): mock_configuration._params["setup"] = { "codecov_url": None, - "codecov_dashboard_url": "test.example.br", + "codecov_dashboard_url": "https://app.codecov.io", } comparison = sample_comparison_for_limited_upload notifier = CommentNotifier( @@ -447,10 +447,10 @@ async def test_notify_upload_limited( assert result.notification_successful assert result.explanation is None expected_message = [ - f"## [Codecov](test.example.br/plan/gh/test-acc9) upload limit reached :warning:", + f"## [Codecov](https://app.codecov.io/plan/gh/test-acc9) upload limit reached :warning:", f"This org is currently on the free Basic Plan; which includes 250 free private repo uploads each rolling month.\ This limit has been reached and additional reports cannot be generated. For unlimited uploads,\ - upgrade to our [pro plan](test.example.br/plan/gh/test-acc9).", + upgrade to our [pro plan](https://app.codecov.io/plan/gh/test-acc9).", f"", f"**Do you have questions or need help?** Connect with our sales team today at ` sales@codecov.io `", ] @@ -507,10 +507,10 @@ async def test_notify_gitlab( "- Partials 0 1 +1 ", "```", "", - "| Flag | Coverage Δ | Complexity Δ | |", + "| [Flag](https://app.codecov.io/gl/joseph-sentry/example-python/pull/1/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", "|---|---|---|---|", - "| integration | `?` | `?` | |", - "| unit | `100.00% <ø> (?)` | `0.00 <ø> (?)` | |", + "| [integration](https://app.codecov.io/gl/joseph-sentry/example-python/pull/1/flags?src=pr&el=flag) | `?` | `?` | |", + "| [unit](https://app.codecov.io/gl/joseph-sentry/example-python/pull/1/flags?src=pr&el=flag) | `100.00% <ø> (?)` | `0.00 <ø> (?)` | |", "", "Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", "", @@ -578,10 +578,10 @@ async def test_notify_new_layout( "- Partials 0 1 +1 ", "```", "", - "| Flag | Coverage Δ | Complexity Δ | |", + "| [Flag](https://app.codecov.io/gh/ThiagoCodecov/example-python/pull/15/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", "|---|---|---|---|", - "| integration | `?` | `?` | |", - "| unit | `100.00% <ø> (?)` | `0.00 <ø> (?)` | |", + "| [integration](https://app.codecov.io/gh/ThiagoCodecov/example-python/pull/15/flags?src=pr&el=flag) | `?` | `?` | |", + "| [unit](https://app.codecov.io/gh/ThiagoCodecov/example-python/pull/15/flags?src=pr&el=flag) | `100.00% <ø> (?)` | `0.00 <ø> (?)` | |", "", "Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", "", @@ -653,18 +653,18 @@ async def test_notify_with_components( "- Partials 0 1 +1 ", "```", "", - "| Flag | Coverage Δ | Complexity Δ | |", + "| [Flag](https://app.codecov.io/gh/codecove2e/example-python/pull/4/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", "|---|---|---|---|", - "| integration | `?` | `?` | |", - "| unit | `100.00% <ø> (?)` | `0.00 <ø> (?)` | |", + "| [integration](https://app.codecov.io/gh/codecove2e/example-python/pull/4/flags?src=pr&el=flag) | `?` | `?` | |", + "| [unit](https://app.codecov.io/gh/codecove2e/example-python/pull/4/flags?src=pr&el=flag) | `100.00% <ø> (?)` | `0.00 <ø> (?)` | |", "", "Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", "", "[see 2 files with indirect coverage changes](https://app.codecov.io/gh/codecove2e/example-python/pull/4/indirect-changes?src=pr&el=tree-more)", "", - "| Components | Coverage Δ | |", + "| [Components](https://app.codecov.io/gh/codecove2e/example-python/pull/4/components?src=pr&el=components) | Coverage Δ | |", "|---|---|---|", - "| go_files | `62.50% <ø> (+12.50%)` | :arrow_up: |", + "| [go_files](https://app.codecov.io/gh/codecove2e/example-python/pull/4/components?src=pr&el=component) | `62.50% <ø> (+12.50%)` | :arrow_up: |", "", "", "", diff --git a/services/notification/notifiers/tests/unit/test_comment.py b/services/notification/notifiers/tests/unit/test_comment.py index 995fc9017..32cf382bb 100644 --- a/services/notification/notifiers/tests/unit/test_comment.py +++ b/services/notification/notifiers/tests/unit/test_comment.py @@ -909,10 +909,10 @@ async def test_build_message( f"- Partials 0 1 +1 ", f"```", f"", - f"| Flag | Coverage Δ | Complexity Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", f"|---|---|---|---|", - f"| integration | `?` | `?` | |", - f"| unit | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", + f"| [integration](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `?` | `?` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", f"", @@ -965,16 +965,16 @@ async def test_build_message_flags_empty_coverage( f"> Merging [#{pull.pullid}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=desc) ({comparison.head.commit.commitid[:7]}) into [master](test.example.br/gh/{repository.slug}/commit/{comparison.base.commit.commitid}?el=desc) ({comparison.base.commit.commitid[:7]}) will **not change** coverage.", f"> The diff coverage is `n/a`.", f"", - "| Flag | Coverage Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | |", "|---|---|---|", - "| eighth | `?` | |", - "| fifth | `∅ <ø> (?)` | |", - "| first | `100.00% <ø> (ø)` | |", - "| fourth | `∅ <ø> (∅)` | |", - "| second | `50.00% <ø> (∅)` | |", - "| seventh | `?` | |", - "| sixth | `100.00% <ø> (?)` | |", - "| third | `∅ <ø> (∅)` | |", + f"| [eighth](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `?` | |", + f"| [fifth](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `∅ <ø> (?)` | |", + f"| [first](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <ø> (ø)` | |", + f"| [fourth](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `∅ <ø> (∅)` | |", + f"| [second](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `50.00% <ø> (∅)` | |", + f"| [seventh](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `?` | |", + f"| [sixth](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <ø> (?)` | |", + f"| [third](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `∅ <ø> (∅)` | |", "", "Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", "", @@ -1036,10 +1036,10 @@ async def test_build_message_more_sections( f"- Partials 0 1 +1 ", f"```", f"", - f"| Flag | Coverage Δ | Complexity Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", f"|---|---|---|---|", - f"| integration | `?` | `?` | |", - f"| unit | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", + f"| [integration](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `?` | `?` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", f"", @@ -1301,10 +1301,10 @@ async def test_build_message_hide_complexity( f"- Partials 0 1 +1 ", f"```", f"", - f"| Flag | Coverage Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | |", f"|---|---|---|", - f"| integration | `?` | |", - f"| unit | `100.00% <100.00%> (?)` | |", + f"| [integration](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `?` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <100.00%> (?)` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", f"", @@ -1375,9 +1375,9 @@ async def test_build_message_no_base_report( f" Partials ? 1 ", f"```", f"", - f"| Flag | Coverage Δ | Complexity Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", f"|---|---|---|---|", - f"| unit | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", f"", @@ -1446,9 +1446,9 @@ async def test_build_message_no_base_commit( f" Partials ? 1 ", f"```", f"", - f"| Flag | Coverage Δ | Complexity Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", f"|---|---|---|---|", - f"| unit | `100.00% <0.00%> (?)` | `0.00% <0.00%> (?%)` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <0.00%> (?)` | `0.00% <0.00%> (?%)` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", f"", @@ -1516,9 +1516,9 @@ async def test_build_message_no_change( f" Partials 1 1 ", f"```", f"", - f"| Flag | Coverage Δ | Complexity Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", f"|---|---|---|---|", - f"| unit | `100.00% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", f"", @@ -1591,10 +1591,10 @@ async def test_build_message_negative_change( f"+ Partials 1 0 -1 ", f"```", f"", - f"| Flag | Coverage Δ | Complexity Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", f"|---|---|---|---|", - f"| integration | `100.00% <ø> (?)` | `0.00 <ø> (?)` | |", - f"| unit | `?` | `?` | |", + f"| [integration](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <ø> (?)` | `0.00 <ø> (?)` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `?` | `?` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", f"", @@ -1786,10 +1786,10 @@ async def test_build_message_show_carriedforward_flags_no_cf_coverage( f"- Partials 0 1 +1 ", f"```", f"", - f"| Flag | Coverage Δ | Complexity Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", f"|---|---|---|---|", - f"| integration | `?` | `?` | |", - f"| unit | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", + f"| [integration](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `?` | `?` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", f"", f"| [Files Changed](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=tree) | Coverage Δ | Complexity Δ | |", f"|---|---|---|---|", @@ -1920,11 +1920,11 @@ async def test_build_message_with_without_flags( f" Partials 68 68 ", f"```", f"", - f"| Flag | Coverage Δ | | *Carryforward flag |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | | *Carryforward flag |", f"|---|---|---|---|", - f"| enterprise | `25.00% <ø> (ø)` | | Carriedforward from [1234567](test.example.br/gh/{repository.slug}/commit/123456789sha) |", - f"| integration | `25.00% <ø> (ø)` | | Carriedforward |", - f"| unit | `25.00% <ø> (ø)` | | |", + f"| [enterprise](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `25.00% <ø> (ø)` | | Carriedforward from [1234567](test.example.br/gh/{repository.slug}/commit/123456789sha) |", + f"| [integration](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `25.00% <ø> (ø)` | | Carriedforward |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `25.00% <ø> (ø)` | | |", f"", f"*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags) to find out more." f"", @@ -1996,11 +1996,11 @@ async def test_build_message_show_carriedforward_flags_has_cf_coverage( f" Partials 68 68 ", f"```", f"", - f"| Flag | Coverage Δ | | *Carryforward flag |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | | *Carryforward flag |", f"|---|---|---|---|", - f"| enterprise | `25.00% <ø> (ø)` | | Carriedforward from [1234567](test.example.br/gh/{repository.slug}/commit/123456789sha) |", - f"| integration | `25.00% <ø> (ø)` | | Carriedforward |", - f"| unit | `25.00% <ø> (ø)` | | |", + f"| [enterprise](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `25.00% <ø> (ø)` | | Carriedforward from [1234567](test.example.br/gh/{repository.slug}/commit/123456789sha) |", + f"| [integration](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `25.00% <ø> (ø)` | | Carriedforward |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `25.00% <ø> (ø)` | | |", f"", f"*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags) to find out more." f"", @@ -2072,9 +2072,9 @@ async def test_build_message_hide_carriedforward_flags_has_cf_coverage( f" Partials 68 68 ", f"```", f"", - f"| Flag | Coverage Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | |", f"|---|---|---|", - f"| unit | `25.00% <ø> (ø)` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `25.00% <ø> (ø)` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more." f"", @@ -2175,9 +2175,9 @@ async def test_build_message_no_flags( f"- Partials 0 1 +1 ", f"```", f"", - f"| Flag | Coverage Δ | Complexity Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", f"|---|---|---|---|", - f"| integration | `?` | `?` | |", + f"| [integration](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `?` | `?` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", f"", @@ -4049,9 +4049,9 @@ async def test_build_message_no_base_commit_new_layout( f" Partials ? 1 ", f"```", f"", - f"| Flag | Coverage Δ | Complexity Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", f"|---|---|---|---|", - f"| unit | `100.00% <0.00%> (?)` | `0.00% <0.00%> (?%)` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <0.00%> (?)` | `0.00% <0.00%> (?%)` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", f"", @@ -4115,9 +4115,9 @@ async def test_build_message_no_base_report_new_layout( f" Partials ? 1 ", f"```", f"", - f"| Flag | Coverage Δ | Complexity Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", f"|---|---|---|---|", - f"| unit | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", f"", @@ -4229,10 +4229,10 @@ async def test_build_message_head_and_pull_head_differ_new_layout( f"- Partials 0 1 +1 ", f"```", f"", - f"| Flag | Coverage Δ | Complexity Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", f"|---|---|---|---|", - f"| integration | `?` | `?` | |", - f"| unit | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", + f"| [integration](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `?` | `?` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", f"", @@ -4304,17 +4304,17 @@ async def test_build_message_head_and_pull_head_differ_with_components( f"- Partials 0 1 +1 ", f"```", f"", - f"| Flag | Coverage Δ | Complexity Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |", f"|---|---|---|---|", - f"| integration | `?` | `?` | |", - f"| unit | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", + f"| [integration](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `?` | `?` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `100.00% <100.00%> (?)` | `0.00 <0.00> (?)` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", f"", - f"| Components | Coverage Δ | |", + f"| [Components](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/components?src=pr&el=components) | Coverage Δ | |", f"|---|---|---|", - f"| go_files | `62.50% <66.67%> (+12.50%)` | :arrow_up: |", - f"| unit_flags | `100.00% <100.00%> (∅)` | |", + f"| [go_files](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/components?src=pr&el=component) | `62.50% <66.67%> (+12.50%)` | :arrow_up: |", + f"| [unit_flags](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/components?src=pr&el=component) | `100.00% <100.00%> (∅)` | |", f"", f"", f"", @@ -4369,9 +4369,9 @@ async def test_build_message_no_patch_or_proj_change( f" Partials 68 68 ", f"```", f"", - f"| Flag | Coverage Δ | |", + f"| [Flag](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flags) | Coverage Δ | |", f"|---|---|---|", - f"| unit | `25.00% <ø> (ø)` | |", + f"| [unit](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/flags?src=pr&el=flag) | `25.00% <ø> (ø)` | |", f"", f"Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more." f"", @@ -4651,10 +4651,10 @@ async def test_write_message_component_section( comparison=comparison, diff=None, changes=None, links={"pull": "urlurl"} ) expected = [ - "| Components | Coverage Δ | |", + "| [Components](urlurl/components?src=pr&el=components) | Coverage Δ | |", "|---|---|---|", - "| go_files | `62.50% <66.67%> (+12.50%)` | :arrow_up: |", - "| py_files | `50.00% <ø> (ø)` | |", + "| [go_files](urlurl/components?src=pr&el=component) | `62.50% <66.67%> (+12.50%)` | :arrow_up: |", + "| [py_files](urlurl/components?src=pr&el=component) | `50.00% <ø> (ø)` | |", ] assert message == expected @@ -4687,9 +4687,9 @@ async def test_write_message_component_section_no_base( comparison=comparison, diff=None, changes=None, links={"pull": "urlurl"} ) expected = [ - "| Components | Coverage Δ | |", + "| [Components](urlurl/components?src=pr&el=components) | Coverage Δ | |", "|---|---|---|", - "| go_files | `62.50% <0.00%> (?)` | |", - "| py_files | `50.00% <0.00%> (?)` | |", + "| [go_files](urlurl/components?src=pr&el=component) | `62.50% <0.00%> (?)` | |", + "| [py_files](urlurl/components?src=pr&el=component) | `50.00% <0.00%> (?)` | |", ] assert message == expected diff --git a/tasks/tests/integration/test_notify_task.py b/tasks/tests/integration/test_notify_task.py index 7cf5f4e50..db37b80d4 100644 --- a/tasks/tests/integration/test_notify_task.py +++ b/tasks/tests/integration/test_notify_task.py @@ -1177,9 +1177,9 @@ async def test_simple_call_status_and_notifiers( " Misses 3 3 ", "```", "", - "| Flag | Coverage Δ | |", + "| [Flag](https://myexamplewebsite.io/gh/test-acc9/test_example/pull/1/flags?src=pr&el=flags) | Coverage Δ | |", "|---|---|---|", - "| unit | `85.00% <ø> (ø)` | |", + "| [unit](https://myexamplewebsite.io/gh/test-acc9/test_example/pull/1/flags?src=pr&el=flag) | `85.00% <ø> (ø)` | |", "", "Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more." "", From 93f8a5b39a1948f9f540514ce9c2a98a2b430cae Mon Sep 17 00:00:00 2001 From: Dana Date: Tue, 15 Aug 2023 17:10:57 +0300 Subject: [PATCH 3/7] renaming function --- services/notification/notifiers/mixins/message/helpers.py | 2 +- services/notification/notifiers/mixins/message/sections.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/services/notification/notifiers/mixins/message/helpers.py b/services/notification/notifiers/mixins/message/helpers.py index 95ffd6232..3088b5249 100644 --- a/services/notification/notifiers/mixins/message/helpers.py +++ b/services/notification/notifiers/mixins/message/helpers.py @@ -135,7 +135,7 @@ def make_patch_only_metrics(before, after, relative, show_complexity, yaml): return "".join(("|", coverage)) -def get_metrics_method(hide_project_coverage): +def get_metrics_function(hide_project_coverage): if hide_project_coverage: metrics = make_patch_only_metrics else: diff --git a/services/notification/notifiers/mixins/message/sections.py b/services/notification/notifiers/mixins/message/sections.py index 1d5efd26a..ae00df2a8 100644 --- a/services/notification/notifiers/mixins/message/sections.py +++ b/services/notification/notifiers/mixins/message/sections.py @@ -20,7 +20,7 @@ diff_to_string, ellipsis, escape_markdown, - get_metrics_method, + get_metrics_function, get_table_header, get_table_layout, make_metrics, @@ -468,12 +468,12 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non if base_report is None: base_report = Report() hide_project_coverage = self.settings.get("hide_project_coverage", False) - metrics = get_metrics_method(hide_project_coverage) + make_metrics_fn = get_metrics_function(hide_project_coverage) files_in_diff = [ ( _diff["type"], path, - metrics( + make_metrics_fn( get_totals_from_file_in_reports(base_report, path) or False, get_totals_from_file_in_reports(head_report, path) or False, _diff["totals"], From 3dbbeed6279b0b6698dc4eae7e52a50de1640a1a Mon Sep 17 00:00:00 2001 From: Giovanni M Guidini <99758426+giovanni-guidini@users.noreply.github.com> Date: Tue, 15 Aug 2023 14:13:09 -0300 Subject: [PATCH 4/7] feat: move `Pull.flare` to storage (#57) This commit is the initial work to move `Pull.flare` to storage. This column has crashed the DB in only 1 occasion in the past. Notice that Pull doesn't have `commit` (because multiple commits refer to the same pull). To deal with that I changed the writing of data to have 2 paths: one with commit, and one without the commit. So pulls data will be written in the repo level (in storage). closes codecov/platform-team#70 --- database/models/core.py | 30 +++++++++++++ database/utils.py | 11 ++++- services/archive.py | 35 +++++++++------ services/tests/test_repository_service.py | 15 ++++--- services/tests/unit/test_archive_service.py | 50 +++++++++++++++++++++ 5 files changed, 121 insertions(+), 20 deletions(-) diff --git a/database/models/core.py b/database/models/core.py index 817d0a026..e5cfb5f3b 100644 --- a/database/models/core.py +++ b/database/models/core.py @@ -331,6 +331,36 @@ def get_head_commit_notifications(self): ) return [] + def get_repository(self): + return self.repository + + def get_commitid(self): + return None + + @property + def external_id(self): + return self.pullid + + @property + def id(self): + return self.id_ + + def should_write_to_storage(self) -> bool: + if self.repository is None or self.repository.owner is None: + return False + is_codecov_repo = self.repository.owner.username == "codecov" + return should_write_data_to_storage_config_check( + master_switch_key="pull_flare", + is_codecov_repo=is_codecov_repo, + repoid=self.repository.repoid, + ) + + _flare = Column("flare", postgresql.JSON) + _flare_storage_path = Column("flare_storage_path", types.Text, nullable=True) + flare = ArchiveField( + should_write_to_storage_fn=should_write_to_storage, default_value={} + ) + class CommitNotification(CodecovBaseModel): __tablename__ = "commit_notifications" diff --git a/database/utils.py b/database/utils.py index d4498d906..aa8aecb6e 100644 --- a/database/utils.py +++ b/database/utils.py @@ -1,7 +1,7 @@ import json import logging from functools import lru_cache -from typing import Any, Callable +from typing import Any, Callable, Optional from shared.storage.exceptions import FileNotInStorageError from shared.utils.ReportEncoder import ReportEncoder @@ -18,16 +18,23 @@ def __subclasscheck__(cls, subclass): and callable(subclass.get_repository) and hasattr(subclass, "get_commitid") and callable(subclass.get_commitid) + and hasattr(subclass, "external_id") ) class ArchiveFieldInterface(metaclass=ArchiveFieldInterfaceMeta): """Any class that uses ArchiveField must implement this interface""" + external_id: str + def get_repository(self): + """Returns the repository object associated with self""" raise NotImplementedError() - def get_commitid(self) -> str: + def get_commitid(self) -> Optional[str]: + """Returns the commitid associated with self. + If no commitid is associated return None. + """ raise NotImplementedError() diff --git a/services/archive.py b/services/archive.py index 2e37389c6..e85d94d69 100644 --- a/services/archive.py +++ b/services/archive.py @@ -20,6 +20,9 @@ class MinioEndpoints(Enum): chunks = "{version}/repos/{repo_hash}/commits/{commitid}/{chunks_file_name}.txt" json_data = "{version}/repos/{repo_hash}/commits/{commitid}/json_data/{table}/{field}/{external_id}.json" + json_data_no_commit = ( + "{version}/repos/{repo_hash}/json_data/{table}/{field}/{external_id}.json" + ) profiling_summary = "{version}/repos/{repo_hash}/profilingsummaries/{profiling_commit_id}/{location}" raw = "v4/raw/{date}/{repo_hash}/{commit_sha}/{reportid}.txt" profiling_collection = "{version}/repos/{repo_hash}/profilingcollections/{profiling_commit_id}/{location}" @@ -200,20 +203,26 @@ def write_json_data_to_storage( *, encoder=ReportEncoder, ): - path = MinioEndpoints.json_data.get_path( - version="v4", - repo_hash=self.storage_hash, - commitid=commit_id, - table=table, - field=field, - external_id=external_id, - ) - stringified_data = json.dumps(data, cls=encoder) - if table == "reports_reportdetails" and not ("meta" in stringified_data): - log.warning( - "Saving files_array data without meta", - extra=dict(commit=commit_id, data=stringified_data, path=path), + if commit_id is None: + # Some classes don't have a commit associated with them + # For example Pull belongs to multiple commits. + path = MinioEndpoints.json_data_no_commit.get_path( + version="v4", + repo_hash=self.storage_hash, + table=table, + field=field, + external_id=external_id, ) + else: + path = MinioEndpoints.json_data.get_path( + version="v4", + repo_hash=self.storage_hash, + commitid=commit_id, + table=table, + field=field, + external_id=external_id, + ) + stringified_data = json.dumps(data, cls=encoder) self.write_file(path, stringified_data) return path diff --git a/services/tests/test_repository_service.py b/services/tests/test_repository_service.py index f62556933..1d43ffb7e 100644 --- a/services/tests/test_repository_service.py +++ b/services/tests/test_repository_service.py @@ -929,7 +929,8 @@ async def test_fetch_and_update_pull_request_information_from_commit_new_pull_co assert res.head == commit.commitid assert res.commentid is None assert res.diff is None - assert res.flare is None + assert res._flare is None + assert res._flare_storage_path is None assert ( res.author == dbsession.query(Owner) @@ -1007,7 +1008,8 @@ async def test_fetch_and_update_pull_request_information_from_commit_existing_pu assert res.head == commit.commitid assert res.commentid is None assert res.diff is None - assert res.flare is None + assert res._flare is None + assert res._flare_storage_path is None assert ( res.author == dbsession.query(Owner) @@ -1091,7 +1093,8 @@ async def test_fetch_and_update_pull_request_multiple_pulls_same_repo( assert res.head == commit.commitid assert res.commentid is None assert res.diff is None - assert res.flare is None + assert res._flare is None + assert res._flare_storage_path is None assert ( res.author == dbsession.query(Owner) @@ -1181,7 +1184,8 @@ async def test_fetch_and_update_pull_request_information_from_commit_different_c assert res.head == commit.commitid assert res.commentid is None assert res.diff is None - assert res.flare is None + assert res._flare is None + assert res._flare_storage_path is None assert ( res.author == dbsession.query(Owner) @@ -1252,7 +1256,8 @@ async def test_fetch_and_update_pull_request_information_no_compared_to( assert res.head is None assert res.commentid is None assert res.diff is None - assert res.flare is None + assert res._flare is None + assert res._flare_storage_path is None assert ( res.author == dbsession.query(Owner) diff --git a/services/tests/unit/test_archive_service.py b/services/tests/unit/test_archive_service.py index 0f65a8696..0d34c7645 100644 --- a/services/tests/unit/test_archive_service.py +++ b/services/tests/unit/test_archive_service.py @@ -86,3 +86,53 @@ def test_write_report_details_to_storage(self, mocker, dbsession): is_already_gzipped=False, reduced_redundancy=False, ) + + def test_write_report_details_to_storage_no_commitid(self, mocker, dbsession): + repo = RepositoryFactory() + dbsession.add(repo) + dbsession.flush() + mock_write_file = mocker.patch.object(MinioStorageService, "write_file") + + data = [ + { + "filename": "file_1.go", + "file_index": 0, + "file_totals": [0, 8, 5, 3, 0, "62.50000", 0, 0, 0, 0, 10, 2, 0], + "session_totals": { + "0": [0, 8, 5, 3, 0, "62.50000", 0, 0, 0, 0, 10, 2], + "meta": {"session_count": 1}, + }, + "diff_totals": None, + }, + { + "filename": "file_2.py", + "file_index": 1, + "file_totals": [0, 2, 1, 0, 1, "50.00000", 1, 0, 0, 0, 0, 0, 0], + "session_totals": { + "0": [0, 2, 1, 0, 1, "50.00000", 1], + "meta": {"session_count": 1}, + }, + "diff_totals": None, + }, + ] + archive_service = ArchiveService(repository=repo) + commitid = None + external_id = "some-uuid4-id" + path = archive_service.write_json_data_to_storage( + commit_id=commitid, + table="reports_reportdetails", + field="files_array", + external_id=external_id, + data=data, + ) + assert ( + path + == f"v4/repos/{archive_service.storage_hash}/json_data/reports_reportdetails/files_array/{external_id}.json" + ) + mock_write_file.assert_called_with( + archive_service.root, + path, + json.dumps(data), + is_already_gzipped=False, + reduced_redundancy=False, + ) From f8aa61130330ade82294e3e6b8c3c365f856cd54 Mon Sep 17 00:00:00 2001 From: Matt Hammerly Date: Tue, 15 Aug 2023 10:16:54 -0700 Subject: [PATCH 5/7] fix: reexport brolly stats rollup so celery can find it outside of tests --- tasks/__init__.py | 1 + tasks/brolly_stats_rollup.py | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/tasks/__init__.py b/tasks/__init__.py index bdc82bd20..b272eddd3 100644 --- a/tasks/__init__.py +++ b/tasks/__init__.py @@ -1,5 +1,6 @@ from app import celery_app from tasks.add_to_sendgrid_list import add_to_sendgrid_list_task +from tasks.brolly_stats_rollup import brolly_stats_rollup_task from tasks.commit_update import commit_update_task from tasks.compute_comparison import compute_comparison_task from tasks.delete_owner import delete_owner_task diff --git a/tasks/brolly_stats_rollup.py b/tasks/brolly_stats_rollup.py index 355e15ef1..0d4a84033 100644 --- a/tasks/brolly_stats_rollup.py +++ b/tasks/brolly_stats_rollup.py @@ -2,7 +2,6 @@ import datetime import json import logging -import uuid # TODO remove import httpx from shared.celery_config import brolly_stats_rollup_task_name From ad044e4bee02e33ae94f6416f796fc13b5560a3c Mon Sep 17 00:00:00 2001 From: Giovanni M Guidini <99758426+giovanni-guidini@users.noreply.github.com> Date: Wed, 16 Aug 2023 10:09:14 -0300 Subject: [PATCH 6/7] fix: use strings as webhook secrets (#66) Fix https://codecov.sentry.io/share/issue/ef92212e1461452c90a16ae188e9c00b/ We were creating UUID4 webhook secrets per repo for gitlab and gitlab-enterprise. When logging the body of the request the json decoder would crash. --- tasks/upload.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tasks/upload.py b/tasks/upload.py index dc3051c92..9d7ccc6ed 100644 --- a/tasks/upload.py +++ b/tasks/upload.py @@ -464,7 +464,7 @@ async def possibly_setup_webhooks(self, commit, repository_service): try: if repository_service.service in ["gitlab", "gitlab_enterprise"]: # we use per-repo webhook secrets in this case - webhook_secret = repository.webhook_secret or uuid.uuid4() + webhook_secret = repository.webhook_secret or str(uuid.uuid4()) else: # service-level config value will be used instead in this case webhook_secret = None From 76c780fbd64bcf0209e17101fb54b6d7cf85c068 Mon Sep 17 00:00:00 2001 From: Giovanni M Guidini <99758426+giovanni-guidini@users.noreply.github.com> Date: Wed, 16 Aug 2023 11:56:52 -0300 Subject: [PATCH 7/7] fix: don't share default values in ArchiveField (#67) Passing an object as the default value to `ArchiveField` (such as `{}`) means that we always return the same reference to multiple objects. So they're shared. (thanks @scott-codecov for pointing that out) So instead we will pass a class and call the class, creating a new object if we have to use it. Noticed I hacked it a little for the default of the defaults to return `None`. We can do the same if we ever need more complex values, or just create a new class. I also moved the log of dbfield and archive_field None to be DEBUG and not INFO. We are getting that quite a lot and it's making support's work hard. --- database/models/core.py | 4 ++-- database/models/reports.py | 2 +- database/tests/unit/test_model_utils.py | 4 +--- database/utils.py | 8 ++++---- 4 files changed, 8 insertions(+), 10 deletions(-) diff --git a/database/models/core.py b/database/models/core.py index e5cfb5f3b..b91830578 100644 --- a/database/models/core.py +++ b/database/models/core.py @@ -238,7 +238,7 @@ def should_write_to_storage(self) -> bool: _report_json_storage_path = Column("report_storage_path", types.Text, nullable=True) report_json = ArchiveField( should_write_to_storage_fn=should_write_to_storage, - default_value={}, + default_value_class=dict, ) @@ -358,7 +358,7 @@ def should_write_to_storage(self) -> bool: _flare = Column("flare", postgresql.JSON) _flare_storage_path = Column("flare_storage_path", types.Text, nullable=True) flare = ArchiveField( - should_write_to_storage_fn=should_write_to_storage, default_value={} + should_write_to_storage_fn=should_write_to_storage, default_value_class=dict ) diff --git a/database/models/reports.py b/database/models/reports.py index 989242c08..c84b8b1f6 100644 --- a/database/models/reports.py +++ b/database/models/reports.py @@ -170,7 +170,7 @@ def _should_write_to_storage(self) -> bool: files_array = ArchiveField( should_write_to_storage_fn=_should_write_to_storage, rehydrate_fn=rehydrate_encoded_data, - default_value=[], + default_value_class=list, ) diff --git a/database/tests/unit/test_model_utils.py b/database/tests/unit/test_model_utils.py index a8abe8f81..abd9f0f67 100644 --- a/database/tests/unit/test_model_utils.py +++ b/database/tests/unit/test_model_utils.py @@ -36,9 +36,7 @@ def __init__( self._archive_field_storage_path = archive_value self.should_write_to_gcs = should_write_to_gcs - archive_field = ArchiveField( - should_write_to_storage_fn=should_write_to_storage, default_value=None - ) + archive_field = ArchiveField(should_write_to_storage_fn=should_write_to_storage) class ClassWithArchiveFieldMissingMethods: commit: Commit diff --git a/database/utils.py b/database/utils.py index aa8aecb6e..8c2aede95 100644 --- a/database/utils.py +++ b/database/utils.py @@ -68,9 +68,9 @@ def __init__( should_write_to_storage_fn: Callable[[object], bool], rehydrate_fn: Callable[[object, object], Any] = lambda self, x: x, json_encoder=ReportEncoder, - default_value=None, + default_value_class=lambda: None, ): - self.default_value = default_value + self.default_value_class = default_value_class self.rehydrate_fn = rehydrate_fn self.should_write_to_storage_fn = should_write_to_storage_fn self.json_encoder = json_encoder @@ -103,14 +103,14 @@ def _get_value_from_archive(self, obj): ), ) else: - log.info( + log.debug( "Both db_field and archive_field are None", extra=dict( object_id=obj.id, commit=obj.get_commitid(), ), ) - return self.default_value + return self.default_value_class() def __get__(self, obj, objtype=None): cached_value = getattr(obj, self.cached_value_property_name, None)