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

Added Travis build status badge in readme #159

Merged
merged 1 commit into from
Sep 25, 2015

Conversation

phillipj
Copy link
Member

Needed sooner or later as someone hopefully writes the first set of tests + addresses the enforcing part of the codestyle linting discussed in #157.

@phillipj
Copy link
Member Author

@nodejs/website shipable or a bad idea?

@fhemberger
Copy link
Contributor

Well, at the moment, we don't have any code to test on Travis CI, and linting may soon be replaced by JS "standard".

@phillipj
Copy link
Member Author

So we dont want to run standard linting with Travis?

@fhemberger
Copy link
Contributor

I think pre-commit is a better way to handle this.

@phillipj
Copy link
Member Author

Hmmm, got to admit I'm no hooks-fan as it will always leave room for someone to not have them in place, even with that pre-commit dependency in place since we cant enforce $ npm install on people.

I would be more than happy to have non-critical things run as a hook, such as only code style checking. standard does more than that though, checking for undefined variables etc which could cause serious issues.

@fhemberger
Copy link
Contributor

Well, if you want to build and test your changes, you have to npm install at one point. Even if someone doesn't do this and their PR gets accepted, the following commit (hopefully) will trigger standard. I have created a PR for that: #172.

At the moment, the site is deployed on commit. So even if you have a "failing" travis badge, this would not stop the site from being deployed, unless we use the deploy hook on Travis CI (@rvagg is it possible to use that for the site without much of a hassle?)

So ideally, we could also combine our two PRs.

@phillipj
Copy link
Member Author

Good point about any commit triggering a deploy anyway. Possibly using Travis webhooks would be great 👍

@rvagg
Copy link
Member

rvagg commented Sep 18, 2015

uh, I've never tried, I have github-webhook that we're using for nodejs.org and iojs.org but don't have anything similar for travis

@fhemberger
Copy link
Contributor

We could use the after_success step (http://docs.travis-ci.com/user/customizing-the-build/) to trigger github-webhook in this case. Necessary secrets can be stored encrypted in .travis.yml: http://docs.travis-ci.com/user/encryption-keys/

@phillipj
Copy link
Member Author

FYI keeping this open as I'm writing some tests for ./scripts/release-post.js ATM which could benefit from this.

@fhemberger
Copy link
Contributor

Cool. If you're done with the tests, can you take a look at the merge conflict, so we can get this merged?

@phillipj
Copy link
Member Author

Absolutely! Will fix it as soon as #193 lands :)

@phillipj
Copy link
Member Author

Absolutely! Will fix it as soon as #193 lands :)

On second thought, I pushed a fix now as we dont need to alter package.json anymore, which avoids further conflicts.

@fhemberger
Copy link
Contributor

As #193 gives us tests to run for the first time (yay!), I'm merging this as well. Thanks!

fhemberger added a commit that referenced this pull request Sep 25, 2015
Added Travis build status badge in readme
@fhemberger fhemberger merged commit 1f6f19d into nodejs:master Sep 25, 2015
@phillipj
Copy link
Member Author

🎉

@mikeal does the nodejs org have Travis CI credentials to hook this up?

@mikeal
Copy link
Contributor

mikeal commented Sep 25, 2015

@phillipj it would be the @nodejs/build WG that would have those credentials.

@phillipj phillipj deleted the travisify branch September 25, 2015 19:10
@phillipj
Copy link
Member Author

Awesome! Could anyone in @nodejs/build with permission to this repo, please enable the Travis CI service?

@rvagg
Copy link
Member

rvagg commented Sep 29, 2015

doesn't need @nodejs/build afaik, everyone here with "Owner" should be able to do this if you just log in to Travis. I've done it now, see if it works.

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