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 coverage settings #802

Closed
wants to merge 1 commit into from
Closed

Conversation

skonto
Copy link
Contributor

@skonto skonto commented Mar 22, 2022

Changes

Fixes the coverage ci settings as discussed here.

/hold for #761

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2022
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: skonto
To complete the pull request process, please assign xtreme-sameer-vohra after the PR has been reviewed.
You can assign the PR to them by writing /assign @xtreme-sameer-vohra in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 22, 2022
@knative-prow-robot
Copy link

Welcome @skonto! It looks like this is your first PR to knative-sandbox/net-kourier 🎉

@knative-prow-robot
Copy link

Hi @skonto. Thanks for your PR.

I'm waiting for a knative-sandbox member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 22, 2022
@nak3
Copy link
Contributor

nak3 commented Mar 22, 2022

Hmm...
The .codecov.yaml is used by github action https://github.com/knative-sandbox/net-kourier/blob/main/.github/workflows/knative-go-test.yaml#L66-L70 but is it true that pull-knative-sandbox-net-kourier-go-coveragealso use it?
Please note that the go-coverage's code is here https://github.com/knative/test-infra/tree/main/tools/coverage/.

Also, if this is proper fix, we need to apply all repos like https://github.com/knative/serving/blob/main/.codecov.yaml#L18 https://github.com/knative/eventing/blob/main/.codecov.yaml#L18 https://github.com/knative/pkg/blob/main/.codecov.yaml#L20.
So if you don't mind, could you bring up your fix against serving or pkg repo? I would like net-kourier repo to follow the parent repos.

Copy link
Contributor

@nak3 nak3 left a comment

Choose a reason for hiding this comment

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

Would like to understand #802 (comment)

@knative-prow
Copy link

knative-prow bot commented Mar 30, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: skonto
To complete the pull request process, please assign xtreme-sameer-vohra after the PR has been reviewed.
You can assign the PR to them by writing /assign @xtreme-sameer-vohra in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@skonto
Copy link
Contributor Author

skonto commented Mar 31, 2022

@nak3 already merged in pkg knative/pkg#2471. Will do for Serving too. Not sure how this file is being picked up by prow, I could dig into it.

@nak3
Copy link
Contributor

nak3 commented Apr 1, 2022

@nak3 already merged in pkg knative/pkg#2471.

I mean why the change - "**/zz_generated.deepcopy.go" # Ignore generated files. needs only for this repository? You didn't change it in pkg repo.

Not sure how this file is being picked up by prow, I could dig into it.

Yes, please. I still cannot believe that the prow uses .codecov.yaml.

@skonto
Copy link
Contributor Author

skonto commented Apr 14, 2022

@nak3 I did ignore whole folders in pkg so no need for the pattern.I dont have access to see the actual reports as the ones failing in #824

@nak3
Copy link
Contributor

nak3 commented Apr 14, 2022

I don't think you ignore whole folders in pkg repo - for example https://github.com/knative/pkg/blob/main/apis/zz_generated.deepcopy.go.

And again, I still cannot believe that the prow uses .codecov.yaml...

@skonto
Copy link
Contributor Author

skonto commented Apr 15, 2022

Ok will try find what prow does as stated before but no promise.

@skonto
Copy link
Contributor Author

skonto commented Apr 15, 2022

Given my comment here we can close this PR. The file is indeed used by codecov, but it seems the coverage tooling has changed. The report here has nothing to do with the previous failed report here that led to this PR.

@nak3
Copy link
Contributor

nak3 commented Apr 15, 2022

Well... You tried to fix this prow issue #761 (comment), right?
You started seeing different coverage.

@nak3 nak3 closed this Apr 15, 2022
@skonto
Copy link
Contributor Author

skonto commented Apr 15, 2022

@nak3

Please note that the go-coverage's code is here https://github.com/knative/test-infra/tree/main/tools/coverage/.

Coverage tool has been deprecated and removed while this PR was open. Check PR here ten days ago. The fix here had the assumption that we still used prow.

Explanation here: knative/test-infra#3155 (comment)

Prow job is not used anymore, codecov is.

Even though the coverage tool has been removed, we should remove the job from here: https://github.com/chizhg/test-infra/blob/f198796824fe7e913af7009f8bacd2d294220684/config/prow/testgrid/testgrid.yaml#L766
as the knative operator project has done: knative/test-infra#3156 a couple of weeks ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants