Skip to content
This repository was archived by the owner on May 5, 2025. It is now read-only.

Fix notifier without any layout - #826

Closed
Swatinem wants to merge 2 commits into
mainfrom
swatinem/fix-no-layout
Closed

Fix notifier without any layout#826
Swatinem wants to merge 2 commits into
mainfrom
swatinem/fix-no-layout

Conversation

@Swatinem

@SwatinemSwatinem commented Oct 28, 2024

Copy link
Copy Markdown
Contributor

This accounts for the case when settings does not have any "layout".

Also cleans up the code a little bit, using newer type annotations, and removing statsd timers.


Should fix ECDN-WORKER-D7G
fixeshttps://github.com/codecov/internal-issues/issues/974

This accounts for the case when `settings` does not have any `"layout"`.
Also cleans up the code a little bit, using newer type annotations, and removing statsd timers.
@Swatinem
Swatinem requested a review from a teamOctober 28, 2024 14:44
@SwatinemSwatinem self-assigned this Oct 28, 2024
@codecov-notifications

codecov-notificationsBot commented Oct 28, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing linesPatch %Lines
.../notification/notifiers/mixins/message/__init__.py96.29%1 Missing ⚠️

Impacted file tree graph

@@ Coverage Diff @@## main #826 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.01% 
==========================================
Files 442 441 -1 Lines 36163 36048 -115 ==========================================
- Hits 35448 35333 -115 
Misses 715 715 
FlagCoverage Δ
integration98.01% <96.29%> (-0.01%)⬇️
unit98.01% <96.29%> (-0.01%)⬇️

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

ComponentsCoverage Δ
NonTestCode95.89% <96.29%> (-0.02%)⬇️
OutsideTasks98.00% <96.29%> (+<0.01%)⬆️
Files with missing linesCoverage Δ
.../notification/notifiers/mixins/message/__init__.py99.07% <96.29%> (-0.93%)⬇️

... and 55 files with indirect coverage changes

@codecov-qa

codecov-qaBot commented Oct 28, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.01%. Comparing base (79f57df) to head (c1f99d8).
Report is 67 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing linesPatch %Lines
.../notification/notifiers/mixins/message/__init__.py96.29%1 Missing ⚠️

Impacted file tree graph

@@ Coverage Diff @@## main #826 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.01% 
==========================================
Files 442 441 -1 Lines 36163 36048 -115 ==========================================
- Hits 35448 35333 -115 
Misses 715 715 
FlagCoverage Δ
integration98.01% <96.29%> (-0.01%)⬇️
unit98.01% <96.29%> (-0.01%)⬇️

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

ComponentsCoverage Δ
NonTestCode95.89% <96.29%> (-0.02%)⬇️
OutsideTasks98.00% <96.29%> (+<0.01%)⬆️
Files with missing linesCoverage Δ
.../notification/notifiers/mixins/message/__init__.py99.07% <96.29%> (-0.93%)⬇️

... and 55 files with indirect coverage changes

@codecov-public-qa

codecov-public-qaBot commented Oct 28, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.01%. Comparing base (79f57df) to head (c1f99d8).
Report is 67 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing linesPatch %Lines
.../notification/notifiers/mixins/message/__init__.py96.29%1 Missing ⚠️

Impacted file tree graph

@@ Coverage Diff @@## main #826 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.01% 
==========================================
Files 442 441 -1 Lines 36163 36048 -115 ==========================================
- Hits 35448 35333 -115 
Misses 715 715 
FlagCoverage Δ
integration98.01% <96.29%> (-0.01%)⬇️
unit98.01% <96.29%> (-0.01%)⬇️

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

ComponentsCoverage Δ
NonTestCode95.89% <96.29%> (-0.02%)⬇️
OutsideTasks98.00% <96.29%> (+<0.01%)⬆️
Files with missing linesCoverage Δ
.../notification/notifiers/mixins/message/__init__.py99.07% <96.29%> (-0.93%)⬇️

... and 55 files with indirect coverage changes

Comment threadservices/notification/notifiers/mixins/message/__init__.py Outdated
Comment threadservices/notification/notifiers/mixins/message/__init__.py
def get_sections(settings: dict) -> list[str]:
layout = (settings.get("layout") or "").strip()
if not layout:
return []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think if you return nothing the PR comment would be empty.
It's better to return the default value in case there is none.

The default value is here (and it should not be lost when the YAML is processed, but merged with the user's option - I think that's the real bug)

@codecov

codecovBot commented Oct 30, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.01%. Comparing base (79f57df) to head (c1f99d8).
Report is 67 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing linesPatch %Lines
.../notification/notifiers/mixins/message/__init__.py96.29%1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@ Coverage Diff @@## main #826 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.01% 
==========================================
Files 442 441 -1 Lines 36163 36048 -115 ==========================================
- Hits 35448 35333 -115 
Misses 715 715 
FlagCoverage Δ
integration98.01% <96.29%> (-0.01%)⬇️
unit98.01% <96.29%> (-0.01%)⬇️

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

ComponentsCoverage Δ
NonTestCode95.89% <96.29%> (-0.02%)⬇️
OutsideTasks98.00% <96.29%> (+<0.01%)⬆️
Files with missing linesCoverage Δ
.../notification/notifiers/mixins/message/__init__.py99.07% <96.29%> (-0.93%)⬇️

... and 55 files with indirect coverage changes

@Swatinem

Copy link
Copy Markdown
ContributorAuthor

Closing this, as this was only happening in QA for our own repos, and
#890 and codecov/codecov-api#980 would fix the cause of the error.

The error is caused by comment: false as configured in the QA org is not being properly overridden when a repo yaml is only setting/overriding very specific fields of comment.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Swatinem@giovanni-guidini