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

Add CircleCI support #55

Merged
merged 1 commit into from Mar 11, 2015
Merged

Add CircleCI support #55

merged 1 commit into from Mar 11, 2015

Conversation

jhersh
Copy link
Contributor

@jhersh jhersh commented Mar 9, 2015

I've added support for CircleCI, a neat CI service we use at work and which I've increasingly been using for my own personal projects. Like Travis, they offer free build containers for FOSS projects.

Here is a sample CircleCI+slather build in one of my projects using my slather fork. The slathering, yea, it doth verily slather, but the build does not show on Coveralls. I am wondering if this is because:

  • The latest CircleCI build for this project is 9, but the latest Travis build is 334, and perhaps Coveralls enforces increasing build number order?
  • A missing access token or some other authorization requirement?
  • Something else?

I would appreciate any suggestions you may have and your help in getting this to the finish line. Thanks!

@ayanonagon
Copy link
Contributor

Yay, thanks for the PR @jhersh! Regarding Coveralls + Circle CI, the folks at Coveralls are super helpful. I filed an issue here a few weeks ago and they helped me diagnose the issue really quickly, so it might be worth asking there too. If anyone else has suggestions, that’d be nice as well.

Thanks again for the PR! :octocat: 🎊

@jhersh
Copy link
Contributor Author

jhersh commented Mar 10, 2015

Update! I believe the missing piece was the ci_access_token, which slather allows you to specify in .slather.yml (it's not clear to me from whence it comes on Travis builds).

That's not ideal -- at least for Coveralls -- as it is a semi-secret key, so I've added that token as an environment variable in my CircleCI project and tweaked my pull request to look for that variable.

My CircleCi project using my slather fork is now posting a build to Coveralls, but it shows up as 'no data' under each of my project files. Closer still...

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 48.63% when pulling 8b33fb1 on jhersh:circleci into 0219ead on venmo:master.

@jhersh
Copy link
Contributor Author

jhersh commented Mar 10, 2015

Woah! I just had a whole bunch of Coveralls builds show up, with per-file coverage and all. Perhaps their queues were backed up for a while?

It looks like everything is showing up properly with the exception of the build branch, commit message, and author name. I think I'll need to add those to the Coveralls JSON payload and then we should be all set.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 48.64% when pulling 2e73f79 on jhersh:circleci into 0219ead on venmo:master.

@ayanonagon
Copy link
Contributor

@jhersh That’s awesome!

@marklarr @kylef Wanna review the Ruby? Everything looks good through my Objective-C/Swift goggles. 😛

@jhersh
Copy link
Contributor Author

jhersh commented Mar 11, 2015

We are close indeed! But I think this is not quite ready just yet - for one, the pull request should only be included in the JSON payload if there is actually a relevant pull request, as otherwise it shows up on Coveralls incorrectly as "pull request #0". I'll try to clean this up soon.

@marklarr
Copy link
Contributor

Cool stuff @jhersh. I'll take a look at the code. Also, here's some relevant talk on the ci_access_token and why it's in the yml file #17 (comment)

PS I'm using SSDataSource in a personal project right now. It's so nice not having to deal with table logic anymore.

@@ -32,6 +53,19 @@ def coveralls_coverage_data
else
raise StandardError, "Environment variable `TRAVIS_JOB_ID` not set. Is this running on a travis build?"
end
elsif ci_service == :circleci
if circleci_job_id
Copy link
Contributor

Choose a reason for hiding this comment

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

@ayanonagon we should probably look into moving these specific cases out into their own methods to cut down on clutter

@marklarr
Copy link
Contributor

looks good! just give a ping when it's ready

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 48.42% when pulling c7e7198 on jhersh:circleci into 8f3cd42 on venmo:master.

@jhersh
Copy link
Contributor Author

jhersh commented Mar 11, 2015

@marklarr Awesome! I'd love to hear your thoughts sometime on SSDataSources :)

I think we are in business! I removed my check for the ci_access_token ENV variable, included the pull request parameter only if there is actually a pull request, and added the most recent commit message to the JSON payload. Things are looking pretty good in Coveralls.

/cc @ayanonagon @BillClinton @KimKardashian

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 48.42% when pulling 4fe8370 on jhersh:circleci into 8f3cd42 on venmo:master.

@marklarr
Copy link
Contributor

@ayanonagon I'm going to turn off the coveralls PR commenter

marklarr added a commit that referenced this pull request Mar 11, 2015
@marklarr marklarr merged commit 1bbe1f9 into SlatherOrg:master Mar 11, 2015
@marklarr
Copy link
Contributor

thanks @jhersh! Honestly, I don't have much to say about SSDataSources yet, and I'd say that's a good thing. I give it some objects and give it some blocks, and I get a table. Quick and easy

@jhersh jhersh deleted the circleci branch March 11, 2015 21:36
@jhersh
Copy link
Contributor Author

jhersh commented Mar 11, 2015

Excellent! We would be keen on a new release of slather as soon as you are able :)

Also I'm to blame for the coveralls spam; I squashed and force pushed a few times 😊

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