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

build: codecov was finding the wrong JSON files #263

Merged
merged 3 commits into from
Feb 5, 2020
Merged

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Feb 4, 2020

codecov was uploading a bunch of the wrong files, let's be more specific.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 4, 2020
@@ -42,7 +42,4 @@ jobs:
- run: npm install
- run: npm test
- run: npm i codecov
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait a second - why do you need to install this here? Instead, should we make sure it's in our package.jsons?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like it'd be okay since it's the repo bot that's trying to run codecov, and not the individual repo package.json. I guess the place that'd be problematic is if we took the output of this process and ran other things that ended up depending on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

So technically, the codecov command line tool could have breaking changes from major version to major version. I don't think it's likely, but it could happen. I'd like to think we should either a.) include the dependency in package.json or b.) specify the version in the npm i call here.

If we go with b, may make sense to use npx too :)

@@ -42,7 +42,4 @@ jobs:
- run: npm install
- run: npm test
- run: npm i codecov
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like it'd be okay since it's the repo bot that's trying to run codecov, and not the individual repo package.json. I guess the place that'd be problematic is if we took the output of this process and ran other things that ended up depending on it.

@@ -42,7 +42,4 @@ jobs:
- run: npm install
- run: npm test
- run: npm i codecov
Copy link
Contributor

Choose a reason for hiding this comment

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

So technically, the codecov command line tool could have breaking changes from major version to major version. I don't think it's likely, but it could happen. I'd like to think we should either a.) include the dependency in package.json or b.) specify the version in the npm i call here.

If we go with b, may make sense to use npx too :)

@codecov
Copy link

codecov bot commented Feb 5, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@c54b988). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #263   +/-   ##
=========================================
  Coverage          ?   87.41%           
=========================================
  Files             ?       15           
  Lines             ?     2559           
  Branches          ?      234           
=========================================
  Hits              ?     2237           
  Misses            ?      309           
  Partials          ?       13

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c54b988...aab65c9. Read the comment docs.

- uses: codecov/codecov-action@v1
with:
token: ${{ secrets.CODECOV_TOKEN }}
- run: ./node_modules/.bin/c8 report --reporter=text-lcov | npx codecov@3 -t ${{ secrets.CODECOV_TOKEN }} --pipe
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised you don't need to explicitly put the with: clause in here. But if it works, 🤷‍♂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like we have access to the secrets set for the repo within the action. I think with is meant more for external actions, which specify the variables they expect using inputs.

@bcoe bcoe merged commit 9b49053 into master Feb 5, 2020
@bcoe bcoe deleted the fix-code-coverage branch February 5, 2020 19:11
feywind added a commit to googleapis/nodejs-pubsub that referenced this pull request Feb 7, 2020
…checks (#878)

* chore: first pass at github actions for some of the repo's precommit checks

* build: adjust for PR comments and what was done in this other PR:

googleapis/repo-automation-bots#263
yoshi-automation added a commit that referenced this pull request Apr 1, 2020
9b49053
commit 9b49053
Author: Benjamin E. Coe <[email protected]>
Date:   Wed Feb 5 11:11:28 2020 -0800

    build: codecov was finding the wrong JSON files (#263)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants