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

chore: disable codecov from v2-release branch #32125

Merged
merged 1 commit into from
Nov 14, 2024
Merged

Conversation

iliapolo
Copy link
Contributor

@iliapolo iliapolo commented Nov 14, 2024

In #32082, we added codecov checks to mergify merge conditions.

This, as originally intended, blocked merging to both the main and the v2-release branches. However, we decided that currently we won't be applying codecov on the v2-release branch because it is redundant.

Ideally, to achieve this, we would remove the conditions only from the mergify configuration that applies to the v2-release branch. But our configuration isn't set up that way right now, it doesn't distinguish between branches.

Luckily, mergify will automatically honor branch protection rules, so we can safely remove the branch dependant conditions from mergify all together, and keep them only in branch protection rules (which we already configured).

Once this PR is merged, I will also remove the codecov branch protection from v2-release (but keep it on main).

Additional Changes

There's some additional codecov related cleanup we need to do so might as well. They are explained as review comments.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@aws-cdk-automation aws-cdk-automation requested a review from a team November 14, 2024 07:22
@github-actions github-actions bot added the p2 label Nov 14, 2024
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 14, 2024
@@ -19,10 +21,10 @@ jobs:
uses: actions/setup-node@v4

- name: Install dependencies
run: cd packages/aws-cdk && yarn install
Copy link
Contributor Author

@iliapolo iliapolo Nov 14, 2024

Choose a reason for hiding this comment

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

Because we don't need to cd here - it does the exact same thing without it.


- name: Build CLI
run: cd packages/aws-cdk && npx lerna run build --scope=aws-cdk
Copy link
Contributor Author

@iliapolo iliapolo Nov 14, 2024

Choose a reason for hiding this comment

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

Because we don't need to cd here - it does the exact same thing without it.

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Nov 14, 2024
@@ -33,4 +35,4 @@ jobs:
directory: packages/aws-cdk/coverage
fail_ci_if_error: true
flags: suite.unit
token: ${{ secrets.CODECOV_TOKEN }}
use_oidc: true
Copy link
Contributor Author

@iliapolo iliapolo Nov 14, 2024

Choose a reason for hiding this comment

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

Switch from token based authentication to OIDC. Verified to work here. This is really nice as it allows us to entirely remove the need for a long lived token.

See https://github.com/codecov/codecov-action?tab=readme-ov-file#using-oidc

@@ -124,10 +124,10 @@ export class CloudWatchLogEventMonitor {
}

private async tick(): Promise<void> {
// excluding from codecoverage because this
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is bizarre but i'm seeing sporadic behavior with respect to coverage of the if statement itself.

Our current report says its uncovered - but how can that be if the other lines of function ARE covered?
Lets disable this until we get to the bottom of it because it introduces sporadic coverage.

Followup to #32107.

@@ -12,7 +12,8 @@ module.exports = {
lines: 81
},
},
"coveragePathIgnorePatterns": [
coveragePathIgnorePatterns: [
...baseConfig.coveragePathIgnorePatterns,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #31702, we inadvertently overridden the base config, instead of adding to it. This caused our test utility files to be included in the coverage report.

See https://app.codecov.io/github/aws/aws-cdk/blob/main/packages%2Faws-cdk%2Ftest%2Fapi%2Ffake-sts.ts

@iliapolo iliapolo added the pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested label Nov 14, 2024
@iliapolo iliapolo marked this pull request as ready for review November 14, 2024 07:32
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.14%. Comparing base (588f1f6) to head (39369c4).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #32125      +/-   ##
==========================================
- Coverage   77.29%   77.14%   -0.15%     
==========================================
  Files         114      105       -9     
  Lines        7627     7163     -464     
  Branches     1360     1311      -49     
==========================================
- Hits         5895     5526     -369     
+ Misses       1550     1457      -93     
+ Partials      182      180       -2     
Flag Coverage Δ
suite.unit 77.14% <ø> (-0.15%) ⬇️

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

Components Coverage Δ
packages/aws-cdk 77.14% <ø> (-0.15%) ⬇️

@aws-cdk-automation aws-cdk-automation dismissed their stale review November 14, 2024 07:34

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-cli-test-run This PR needs CLI tests run against it. label Nov 14, 2024
@iliapolo iliapolo changed the title chore: remove codecov from mergify chore: disable codecov from v2-release branch Nov 14, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 39369c4
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 14, 2024
@mrgrain mrgrain added the pr/do-not-merge This PR should not be merged at this time. label Nov 14, 2024
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 14, 2024
@iliapolo iliapolo merged commit f8415c0 into main Nov 14, 2024
58 of 65 checks passed
@iliapolo iliapolo deleted the epolon/codecov-fixes branch November 14, 2024 08:28
Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
contribution/core This is a PR that came from AWS. p2 pr/do-not-merge This PR should not be merged at this time. pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants