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

Fix notifier without any layout #826

Closed
wants to merge 2 commits into from
Closed

Conversation

Swatinem
Copy link
Contributor

@Swatinem Swatinem commented Oct 28, 2024

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
fixes https://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 team October 28, 2024 14:44
@Swatinem Swatinem self-assigned this Oct 28, 2024
@codecov-notifications
Copy link

codecov-notifications bot commented Oct 28, 2024

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 lines Patch % Lines
.../notification/notifiers/mixins/message/__init__.py 96.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              
Flag Coverage Δ
integration 98.01% <96.29%> (-0.01%) ⬇️
unit 98.01% <96.29%> (-0.01%) ⬇️

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

Components Coverage Δ
NonTestCode 95.89% <96.29%> (-0.02%) ⬇️
OutsideTasks 98.00% <96.29%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
.../notification/notifiers/mixins/message/__init__.py 99.07% <96.29%> (-0.93%) ⬇️

... and 55 files with indirect coverage changes

@codecov-qa
Copy link

codecov-qa bot commented Oct 28, 2024

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 lines Patch % Lines
.../notification/notifiers/mixins/message/__init__.py 96.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              
Flag Coverage Δ
integration 98.01% <96.29%> (-0.01%) ⬇️
unit 98.01% <96.29%> (-0.01%) ⬇️

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

Components Coverage Δ
NonTestCode 95.89% <96.29%> (-0.02%) ⬇️
OutsideTasks 98.00% <96.29%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
.../notification/notifiers/mixins/message/__init__.py 99.07% <96.29%> (-0.93%) ⬇️

... and 55 files with indirect coverage changes

Copy link

codecov-public-qa bot commented Oct 28, 2024

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 lines Patch % Lines
.../notification/notifiers/mixins/message/__init__.py 96.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              
Flag Coverage Δ
integration 98.01% <96.29%> (-0.01%) ⬇️
unit 98.01% <96.29%> (-0.01%) ⬇️

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

Components Coverage Δ
NonTestCode 95.89% <96.29%> (-0.02%) ⬇️
OutsideTasks 98.00% <96.29%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
.../notification/notifiers/mixins/message/__init__.py 99.07% <96.29%> (-0.93%) ⬇️

... and 55 files with indirect coverage changes

services/notification/notifiers/mixins/message/__init__.py Outdated Show resolved Hide resolved
def get_sections(settings: dict) -> list[str]:
layout = (settings.get("layout") or "").strip()
if not layout:
return []
Copy link
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)

Copy link

codecov bot commented Oct 30, 2024

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 lines Patch % Lines
.../notification/notifiers/mixins/message/__init__.py 96.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              
Flag Coverage Δ
integration 98.01% <96.29%> (-0.01%) ⬇️
unit 98.01% <96.29%> (-0.01%) ⬇️

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

Components Coverage Δ
NonTestCode 95.89% <96.29%> (-0.02%) ⬇️
OutsideTasks 98.00% <96.29%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
.../notification/notifiers/mixins/message/__init__.py 99.07% <96.29%> (-0.93%) ⬇️

... and 55 files with indirect coverage changes

@Swatinem
Copy link
Contributor Author

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.

@Swatinem Swatinem closed this Nov 15, 2024
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