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

Feat(build seq) codecov, use bash instead of npm module. #1078

Closed
wants to merge 1 commit into from

Conversation

sukrosono
Copy link
Contributor

Fix #1077

@sukrosono sukrosono requested a review from a team as a code owner November 2, 2017 23:52
@codecov
Copy link

codecov bot commented Nov 2, 2017

Codecov Report

Merging #1078 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1078   +/-   ##
======================================
  Coverage    93.6%   93.6%           
======================================
  Files          32      32           
  Lines        1626    1626           
  Branches      393     393           
======================================
  Hits         1522    1522           
  Misses        104     104

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 2eafe73...2897512. Read the comment docs.

@keithamus
Copy link
Member

keithamus commented Nov 3, 2017

Hey @brutalcrozt thanks for the PR.

I feel really uncomfortable downloading and executing aribtrary bash on my local machine, and on CI (where we have secret tokens like account passwords). The 300k is only downloaded for developers/ci (where it is also heavily cached), so I think it's adequete to keep it. I'll close this PR, you did the right thing in the original PR, and changing to running arbitrary bash would be a bad move IMO.

@keithamus keithamus closed this Nov 3, 2017
@astorije
Copy link
Member

astorije commented Nov 3, 2017

Something I often see is adding the dependency in the build config itself and not package.json:

install:
  - npm install -g codecov
script:
  - ...
  - codecov

That would solve what you were trying to solve, @brutalcrozt, without relying on a bash download.


In fact, considering codecov is currently not installed -globally, how come running codecov works? I'd expect to see ./node_modules/.bin/codecov in there instead, unless Travis runs their script section from ./node_modules/.bin itself.

@keithamus
Copy link
Member

keithamus commented Nov 3, 2017

how come running codecov works

I believe Travis have ./node_modules/.bin in the PATH.

...adding the dependency in the build config...

We could make travis install codecov independantly, but I would like to draw complexity out of our travis scripts and into tooling that can work agnostic of CI - especially as I'd like to add appveyor into our toolchain for testing IE without saucelabs, so package.json feels right to me. Other should comment though, we should reach a consensus.

@meeber
Copy link
Contributor

meeber commented Nov 3, 2017

I like the self-documenting nature of keeping the dev dependency in package.json. I'd like to keep it as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants