build: set fail_ci_if_error to true

[arbrandes]

In an effort to standardize codecov across the org, flip fail_ci_if_error to true as per https://openedx.atlassian.net/wiki/spaces/COMM/pages/3438280709/Adding+Codecov.

See openedx/wg-frontend#179

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.00%. Comparing base (cdc351c) to head (66a6f8f).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1099   +/-   ##
=======================================
  Coverage   86.00%   86.00%           
=======================================
  Files         381      381           
  Lines        7796     7796           
  Branches     1902     1900    -2     
=======================================
  Hits         6705     6705           
  Misses       1037     1037           
  Partials       54       54           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[adamstankiewicz]

@arbrandes We had intentionally kept this fail_ci_if_error as false for the Enterprise MFEs because we, as maintainers, would prefer to not have our PRs (e.g., potentially an urgent bug fix) blocked only due to CodeCov being down and/or broken, especially when we deploy directly from master.

# NOTE: Change this to 'false' if you want codecov failures to still quietly let the
#       workflow pass. We only recommend this in cases where the workflow passing
#       is more important than codecov working; for example, package releases. At
#       least one workflow in your repo should have `fail_ci_if_error: true`; otherwise,
#       codecov may break and nobody will notice.

At least one workflow in your repo should have fail_ci_if_error: true; otherwise, codecov may break and nobody will notice.

I agree not having observability into when CodeCov is broken/down is not great, but I feel potentially urgent bug fixes and other contributions getting blocked from merging because CodeCov happens to be down isn't great either.

[question/suggestion] Is there an alternative here where we could make the steps defined in this ci.yml workflow part of the required status checks prior to merging a PR, but abstract the coverage step out into its own coverage.yml workflow (or similar)? That is, where fail_ci_if_error could be set to true, but without it being part of the status checks required prior to merge?

A desired outcome (IMHO):

• Successful upload/execution of CodeCov results in project/patch coverage diffs in CI. These coverage diffs may fail CI should they introduce a drop in coverage exceeding any configured coverage thresholds (i.e., defined via codecov.yml).
• Failure to upload/execute CodeCov is reported as a failure in CI, but does not block PRs from merging.

tl;dr; It makes sense to fail CI and block PRs from merging when they cause a drop in coverage exceeding any specified thresholds in codecov.yml, but ideally not simply because CodeCov is temporarily down / broken / mis-configured.

cc @brobro10000

[arbrandes]

I'm sure you realize that anything that makes codecov optional - whatever the reason - means we're admitting that coverage reductions/omissions are ok. This is not something we want to encourage as a project. Your suggested alternative is better than what we have now, but it still falls under the category of "sometimes we're blindly letting coverage fails through".

That said, we don't want to gate PRs on possibly flaky external services such as codecov for the very reasons you outline. (The token snafu with v4 is just the latest of the headaches.) So the mid-term plan is to find tools to do it locally. We're open to suggestions, particularly in the frontend world.

In the meantime, given this repo is actively maintained, I'll leave it up to you. (After all, the maintainer is responsible for coverage.yml anyway.) Closing!