3.B Investigation

[codecovdesign]

revisit open PRs and determine how to proceed:
- feat: notify_error default=True for repos created after a certain date (shared#320)
- feat: make notify_error default to true in config worker (worker#601)

Latest design

[trent-codecov]

@joseph-sentry - resurrect these PRs.

@adrian-codecov - work with @joseph-sentry on these PRs, review them, work with Kyle as needed.

@codecovdesign - the desired end state is merging these correct?

[codecovdesign]

the desired end state is merging these correct?

yes, though also revisiting them and confirming our understanding of what they do + how they may be leveraged for next iteration. I'll setup a quick sync with @joseph-sentry and @adrian-codecov to discuss!

notes with @joseph-sentry and @adrian-codecov on 11/20

• what are these PRs, what's the context around them?
  • feat: make notify_error default to true in config worker#601 in the scenario that an error is detected during processing, this PR would communicate this so user is aware. the issue is if there are users that upload reports that fail the processing (IF this was merged this may break the UX of receiving a report)
    • could there be an intermediary state of showing the error AND the report? cc @joseph-sentry
    • this raises the question of could a similar intermediary state be shown during processing cc @adrian-codecov
  • @codecovdesign design ToDo ✅ (WIP) show what these intermiary states look like and setup follow up sync to include (@drazisil-codecov)

[codecovdesign]

sync with @eliatcodecov @rohan-at-sentry @Adal3n3 @joseph-sentry @drazisil-codecov @vlad-ko

• eli: consider IF we could identify that the lines related to changes (patch) we know it's covered, then let's in that scenario tell the user.
  • consider if there is an error, they still may not be able to resolve that and then they are stuck
  • can this be done / feasible and/or requires investigation?
  • this applies to both the error / partial portion
• joe: consider the status checks where there is a pending status
  • since this is the case, the status check is pending and we could consider not showing anything until an error state OR full
  • IF error occurs, then show copy w/ reporting we have (when patch is revealed)
    - IF CI has error then codecov can stop (fail_on_error) (consideration?)
  • IF no errors occur, await full report
    • or partial show IF patch lines arrive as covered (either scenario)
• joey: consider scenario when testing is in multiple environment, it could be that patch appears covered in X area, but not Y – considering the flags
• patch portion could be omitted from v1
  • IF no error, status in pending then don't have a comment and instead await
  • IF an error, show error and NOT show the report portion
    - + discovery on the some follow ups here (consider scenario where patch is covered)
• fail_on_CI
  • should this be a default?

[codecovdesign]

sync @joseph-sentry @adrian-codecov

goals for changes:

• IF no error, status in pending then don't have a comment and instead await full reapor
• IF an error, show error and show the report portion

sync notes

• regarding the behavior change (await full reporting), @adrian-codecov: will check, but it appears we have this behavior built in when we can determine "done" – so leveraging logic and updating to only show report it reached this status
  • also will investigate/confirm scenario of whether multiple email updates are made to user
  • https://github.com/codecov/worker/blob/bf567a54ea594558eae1bf4e0a872ba1f4f2f8f1/services/processing/state.py#L62
• regarding the error PR and update to show error AND reporting, there is a thing called "notification context"
  • for now show generic messaging (later can follow with x of y upload failing)
  • @adrian-codecov sync with @suejung-sentry to see how commit page sources errors

[adrian-codecov]

This investigation is done, the summary and specifics of the findings are here https://www.notion.so/sentry/Approach-1618b10e4b5d8037a13ced3641eb3afe