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

Run coveralls only on parent repo #4699

Merged
merged 1 commit into from
Nov 1, 2017
Merged

Conversation

andig
Copy link
Contributor

@andig andig commented Aug 25, 2017

...otherwise each build in forks is broken which is highly confusing.

@simonbrunel
Copy link
Member

I'm not sure if I would simply silent any error instead:

- cat ./coverage/lcov.info | ./node_modules/.bin/coveralls 2>/dev/null

Then it still working with forks integrating with their own coveralls accounts.

@andig
Copy link
Contributor Author

andig commented Sep 20, 2017

Sorry, I had lost this PR from my radar. That would silence the error but would also do so for your regular build. If bin/coveralls is only responsible for sending the report to a different server (?) and not doing any validity checking by itself that should be ok?

Might also do a

- cat ./coverage/lcov.info | ./node_modules/.bin/coveralls || true

That should still print the error if there is one but not fail.

@etimberg etimberg added this to the Version 2.8 milestone Sep 29, 2017
@simonbrunel
Copy link
Member

@andig I like your version better (yes bin/coveralls is only responsible for sending the report on their server, nothing critical if it fails)

@benmccann
Copy link
Contributor

@andig were you going to update this PR to match your last comment?

@andig
Copy link
Contributor Author

andig commented Nov 1, 2017

were you going to update this PR to match your last comment?

Sorry, I was somehow under the impression that it still needed discussion. Updated PR.

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks @andig!

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

Successfully merging this pull request may close these issues.

4 participants