Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: commit status message (for BA) #610

Merged
merged 4 commits into from
Aug 14, 2024

Conversation

giovanni-guidini
Copy link
Contributor

Introduces the message strategy to send commit statuses for BA.
Does NOT activate these statuses yet.

There were some intermediary changes, please check the other commits for more details.

@codecov-qa
Copy link

codecov-qa bot commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 99.39271% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.68%. Comparing base (5324736) to head (4ab9c5e).
Report is 5 commits behind head on main.

✅ All tests successful. No failed tests found.

Files Patch % Lines
...ndle_analysis/new_notify/contexts/commit_status.py 96.29% 3 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #610      +/-   ##
==========================================
+ Coverage   97.59%   97.68%   +0.08%     
==========================================
  Files         431      438       +7     
  Lines       36147    36601     +454     
==========================================
+ Hits        35279    35754     +475     
+ Misses        868      847      -21     
Flag Coverage Δ
integration 97.68% <99.39%> (+0.08%) ⬆️
latest-uploader-overall 97.68% <99.39%> (+0.08%) ⬆️
unit 97.68% <99.39%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 94.99% <97.97%> (+0.19%) ⬆️
OutsideTasks 97.92% <99.39%> (+0.09%) ⬆️
Files Coverage Δ
services/bundle_analysis/comparison.py 100.00% <100.00%> (ø)
services/bundle_analysis/new_notify/conftest.py 100.00% <100.00%> (ø)
...es/bundle_analysis/new_notify/contexts/__init__.py 100.00% <100.00%> (+1.08%) ⬆️
...ces/bundle_analysis/new_notify/contexts/comment.py 98.33% <100.00%> (-0.16%) ⬇️
.../new_notify/contexts/tests/test_comment_context.py 100.00% <100.00%> (ø)
...otify/contexts/tests/test_commit_status_context.py 100.00% <100.00%> (ø)
...nalysis/new_notify/contexts/tests/test_contexts.py 100.00% <ø> (ø)
services/bundle_analysis/new_notify/helpers.py 100.00% <100.00%> (+1.96%) ⬆️
...ces/bundle_analysis/new_notify/messages/comment.py 100.00% <ø> (ø)
...ndle_analysis/new_notify/messages/commit_status.py 100.00% <100.00%> (ø)
... and 4 more

... and 15 files with indirect coverage changes

Copy link

codecov-public-qa bot commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 99.39271% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.68%. Comparing base (5324736) to head (4ab9c5e).
Report is 5 commits behind head on main.

✅ All tests successful. No failed tests found.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #610      +/-   ##
==========================================
+ Coverage   97.59%   97.68%   +0.08%     
==========================================
  Files         431      438       +7     
  Lines       36147    36601     +454     
==========================================
+ Hits        35279    35754     +475     
+ Misses        868      847      -21     
Flag Coverage Δ
integration 97.68% <99.39%> (+0.08%) ⬆️
latest-uploader-overall 97.68% <99.39%> (+0.08%) ⬆️
unit 97.68% <99.39%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 94.99% <97.97%> (+0.19%) ⬆️
OutsideTasks 97.92% <99.39%> (+0.09%) ⬆️
Files Coverage Δ
services/bundle_analysis/comparison.py 100.00% <100.00%> (ø)
services/bundle_analysis/new_notify/conftest.py 100.00% <100.00%> (ø)
...es/bundle_analysis/new_notify/contexts/__init__.py 100.00% <100.00%> (+1.08%) ⬆️
...ces/bundle_analysis/new_notify/contexts/comment.py 98.33% <100.00%> (-0.16%) ⬇️
.../new_notify/contexts/tests/test_comment_context.py 100.00% <100.00%> (ø)
...otify/contexts/tests/test_commit_status_context.py 100.00% <100.00%> (ø)
...nalysis/new_notify/contexts/tests/test_contexts.py 100.00% <ø> (ø)
services/bundle_analysis/new_notify/helpers.py 100.00% <100.00%> (+1.96%) ⬆️
...ces/bundle_analysis/new_notify/messages/comment.py 100.00% <ø> (ø)
...ndle_analysis/new_notify/messages/commit_status.py 100.00% <100.00%> (ø)
... and 4 more

... and 15 files with indirect coverage changes

@giovanni-guidini giovanni-guidini force-pushed the gio/bs-commit-status/notification-settings branch from 1946182 to 9611ce2 Compare August 13, 2024 09:10
Base automatically changed from gio/bs-commit-status/notification-settings to main August 13, 2024 09:35
The `ComparisonLoader` depended on a `EnrichedPull` object to be created.
This made sense in the PR Comment context where there's always a pull.
For the commit status we might not have a Pull, in which case we will default to
comparing to the current commit's parent.

This is still a comparison nonetheless, so most of the loader is the same.
I'm just changing how it is created and adding a `from_EnrichedPull` classmethod.

This is to be used in the bundle_analysis commit status
Creates the commit status context for bundle analysis.
I moved some things around in the previous commit and this one to better accomodate changes and reuse code.

Pretty similar to the PRCommentContext, but has some more details. And the pulls can be `None` (which causes the comparison to have 2 possible sources)
@giovanni-guidini giovanni-guidini force-pushed the gio/ba-commit-status/context-and-message branch from 631277b to 178a7f5 Compare August 13, 2024 09:39
Introduces the message strategy to send commit statuses for BA.
Does _NOT_ activate these statuses yet.
@giovanni-guidini giovanni-guidini force-pushed the gio/ba-commit-status/context-and-message branch from 178a7f5 to 2ba43c9 Compare August 13, 2024 10:36
@codecov-notifications
Copy link

codecov-notifications bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 99.39271% with 3 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files Patch % Lines
...ndle_analysis/new_notify/contexts/commit_status.py 96.29% 3 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #610      +/-   ##
==========================================
+ Coverage   97.59%   97.68%   +0.08%     
==========================================
  Files         431      438       +7     
  Lines       36147    36601     +454     
==========================================
+ Hits        35279    35754     +475     
+ Misses        868      847      -21     
Flag Coverage Δ
integration 97.68% <99.39%> (+0.08%) ⬆️
latest-uploader-overall 97.68% <99.39%> (+0.08%) ⬆️
unit 97.68% <99.39%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 94.99% <97.97%> (+0.19%) ⬆️
OutsideTasks 97.92% <99.39%> (+0.09%) ⬆️
Files Coverage Δ
services/bundle_analysis/comparison.py 100.00% <100.00%> (ø)
services/bundle_analysis/new_notify/conftest.py 100.00% <100.00%> (ø)
...es/bundle_analysis/new_notify/contexts/__init__.py 100.00% <100.00%> (+1.08%) ⬆️
...ces/bundle_analysis/new_notify/contexts/comment.py 98.33% <100.00%> (-0.16%) ⬇️
.../new_notify/contexts/tests/test_comment_context.py 100.00% <100.00%> (ø)
...otify/contexts/tests/test_commit_status_context.py 100.00% <100.00%> (ø)
...nalysis/new_notify/contexts/tests/test_contexts.py 100.00% <ø> (ø)
services/bundle_analysis/new_notify/helpers.py 100.00% <100.00%> (+1.96%) ⬆️
...ces/bundle_analysis/new_notify/messages/comment.py 100.00% <ø> (ø)
...ndle_analysis/new_notify/messages/commit_status.py 100.00% <100.00%> (ø)
... and 4 more

... and 15 files with indirect coverage changes

@giovanni-guidini giovanni-guidini requested a review from a team August 13, 2024 10:44
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 99.39271% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.74%. Comparing base (5324736) to head (4ab9c5e).
Report is 13 commits behind head on main.

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

✅ All tests successful. No failed tests found.

Files Patch % Lines
...ndle_analysis/new_notify/contexts/commit_status.py 96.29% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #610      +/-   ##
==========================================
+ Coverage   97.63%   97.74%   +0.10%     
==========================================
  Files         466      473       +7     
  Lines       37353    38180     +827     
==========================================
+ Hits        36471    37319     +848     
+ Misses        882      861      -21     
Flag Coverage Δ
integration 91.38% <99.39%> (-6.22%) ⬇️
latest-uploader-overall 97.68% <99.39%> (+0.08%) ⬆️
unit 97.68% <99.39%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 95.13% <97.97%> (+0.25%) ⬆️
OutsideTasks 97.92% <99.39%> (+0.09%) ⬆️
Files Coverage Δ
services/bundle_analysis/comparison.py 100.00% <100.00%> (ø)
services/bundle_analysis/new_notify/conftest.py 100.00% <100.00%> (ø)
...es/bundle_analysis/new_notify/contexts/__init__.py 100.00% <100.00%> (+1.08%) ⬆️
...ces/bundle_analysis/new_notify/contexts/comment.py 98.33% <100.00%> (-0.16%) ⬇️
.../new_notify/contexts/tests/test_comment_context.py 100.00% <100.00%> (ø)
...otify/contexts/tests/test_commit_status_context.py 100.00% <100.00%> (ø)
...nalysis/new_notify/contexts/tests/test_contexts.py 100.00% <ø> (ø)
services/bundle_analysis/new_notify/helpers.py 100.00% <100.00%> (+1.96%) ⬆️
...ces/bundle_analysis/new_notify/messages/comment.py 100.00% <ø> (ø)
...ndle_analysis/new_notify/messages/commit_status.py 100.00% <100.00%> (ø)
... and 4 more

... and 15 files with indirect coverage changes

This change has been scanned for critical changes. Learn more

Copy link
Contributor

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, left some minor comments

self._head_commit: Commit | None = head_commit

@classmethod
def from_EnrichedPull(cls, pull: EnrichedPull) -> "ComparisonLoader":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def from_EnrichedPull(cls, pull: EnrichedPull) -> "ComparisonLoader":
def from_EnrichedPull(cls, pull: EnrichedPull) -> ComparisonLoader:

I believe this is now valid syntax in Python 3.12

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My IDE complains that it is not defined. And the container fails with

NameError: name 'ComparisonLoader' is not defined

So maybe not? I might be missing something

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I just checked that it is still behind the special from __future__ import annotations import.
I was just used to that from working on Sentry.

Now I’m not quite sure if shorthand Union / Optional syntax TypeA | TypeB | None is also still behind that special import, or if that is readily available now. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also found out that from typing import Self exists, and that it seems to be even a better fit when inheriting from base classes, as it will just forward the concrete type that methods are being called on. And its less to type (apart from the import)

* Remove prints
* Refactor `build_context` chaining method calls
* In-line check of "is message cached" in the notification method
* Early return if message was already sent
* Remove leading space from template
@giovanni-guidini giovanni-guidini force-pushed the gio/ba-commit-status/context-and-message branch from 93928b3 to 4ab9c5e Compare August 14, 2024 11:01
@giovanni-guidini giovanni-guidini added this pull request to the merge queue Aug 14, 2024
Merged via the queue into main with commit d5c5cb0 Aug 14, 2024
26 of 27 checks passed
@giovanni-guidini giovanni-guidini deleted the gio/ba-commit-status/context-and-message branch August 14, 2024 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants