Skip to content

Remove Travis CI and Coveralls. Add Codecov as coverage reporting tool.#39

Merged
parkerziegler merged 1 commit intov1from
task/remove-travis
Apr 6, 2019
Merged

Remove Travis CI and Coveralls. Add Codecov as coverage reporting tool.#39
parkerziegler merged 1 commit intov1from
task/remove-travis

Conversation

@parkerziegler
Copy link
Copy Markdown
Contributor

@parkerziegler parkerziegler commented Apr 5, 2019

This PR removes Travis CI as our CI provider, adds Codecov as our coverage reporting tool, and modifies our .sail.yml to work with environment variables. There are still some kinks to work out with Codecov – right now we aren't actually getting reporting showing up in their dashboards. I'm suspecting some of this is because we're introducing this change on a branch off a branch off of master. There are some issues with Codecov not having a "base report" that I've attempted to amend via by using their bash uploader, but it's not quite behaving properly. I have a support ticket out to them now that we'll hopefully get some word on soon. In the meantime, our Sail builds are passing nicely 👍.

@parkerziegler parkerziegler marked this pull request as ready for review April 6, 2019 02:29
@parkerziegler parkerziegler requested a review from kiraarghy April 6, 2019 02:29
Copy link
Copy Markdown
Contributor

@kiraarghy kiraarghy left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

- yarn
args:
- codecov
- --token=$(CODECOV_TOKEN)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Isn’t this supposed to be ${} and not $() or is it some domain specific interpolation that’s not shell?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's originally what I thought too. Then I read Sail's docs on environment variables: https://sail.ci/docs/environment-variables. Check out the "Use Environment Variables in Task Properties" section, apparently they do interpolation using $().

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Interesting! So, does that mean they're pre-processing the interpolation to be something like:

$ export CODECOV_TOKEN="hi"
$ yarn codecov --token=hi

?

Because if this was bash with $(STRING) the STRING would be executed as a command. Curious if doing - --token=${CODECOV_TOKEN} produces

$ export CODECOV_TOKEN="hi"
$ yarn codecov --token=${CODECOV_TOKEN}

.. which seems like it should work too...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My guess is they must be doing something like the above. I believe I ran an experiment where I had the CI run echo ${CODECOV_TOKEN} and it worked. The key to all of this is ensuring that the env key on the task is specified so that the environment variable you want to use is mounted: https://github.com/FormidableLabs/reason-urql/blob/v1/.sail.yml#L28-L29. Basically, the steps seem to be:

  • Add env variables through Sail's app (app.sail.ci).
  • Specify which variables to bring into scope in which task (using the env config option as shown above).
  • Interpolate the variable using $().

Sail does have a few other drawbacks from my research thus far.

  • You can't see the build logs in their UI (app.sail.ci). The only way to see them is via the "Checks" tab on a PR. Which means you just don't get logs for non-PR'ed builds.
  • Their environment variables UI seems badly broken. You can't actually save environment variables without opening the dev tools and bringing the Save button into view (or praying you tab navigate to the right out of view button 😬).
    SailUI
  • Their deletion of environment variables doesn't seem to be working. If you delete variables in the UI and then refresh the page, your variables will still appear (and work in CI, meaning they're being persisted).
    SailNoDeletion

The CI itself seems to be running fine, but in the longer run I probably will advocate for us to migrate to Circle if these issues persist.

@ryan-roemer
Copy link
Copy Markdown

Might want to review inspectpack which also does codecov but without any token vars

@parkerziegler
Copy link
Copy Markdown
Contributor Author

parkerziegler commented Apr 6, 2019

Yeah, I took a look at both inspectpack and react-ssr-prepass. It looks like Codecov can auto-detect Circle and Travis, so a token isn't necessary when using those CIs (and hence why we need app.sail.ci in our codecov.yml). What is stumping me is that I tried running this build with Travis still enabled: https://travis-ci.org/FormidableLabs/reason-urql/builds/516009533. It reaches the codecov step just fine and even outputs a success state in the build log. However, if you follow the link to the Codecov report, it responds with an opaque "There was an error processing coverage reports." I've checked the common issues, and the only one I think it might be are the coverage reports not resolving, but I can't find a way to debug that with Codecov.

@parkerziegler
Copy link
Copy Markdown
Contributor Author

parkerziegler commented Apr 6, 2019

image

This is probably what confuses me the most – the "Missing Base Report" notification (available under the Pulls tab in Codecov). I may try moving the default branch to v1 for now and seeing if using the bash uploader to upload a report to v1 helps generate the proper report. Like I mentioned, I have an email out to Codecov and will hopefully get some help from them.

@parkerziegler parkerziegler merged commit b03ad10 into v1 Apr 6, 2019
@parkerziegler parkerziegler deleted the task/remove-travis branch April 6, 2019 17:17
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.

3 participants