Skip to content
This repository was archived by the owner on Apr 30, 2021. It is now read-only.

Conversation

@sethladd
Copy link
Contributor

No description provided.

@sethladd
Copy link
Contributor Author

only run travis when PR updates, or when merging to master
@pq
Copy link
Contributor

pq commented Aug 17, 2015

I don't feel qualified to vet the specifics of how this works since I don't know anything about appveyor but I love where this is headed! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Why only test master? I actually found it handy to have Travis test all branches, since that let us do experimental work on a branch and see the results immediately, without having to do pull requests to trigger testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a misleading line. It's not what you think it means. :)

Traditionally, Travis will kick off two builds when you push a PR, or update a PR: one build for the branch and one build for the PR. This is duplicate work.

This config line here implicitly says "run a test when you push a PR, and when master changes"

Copy link
Contributor

Choose a reason for hiding this comment

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

Um, actually, that's exactly what I thought it meant. I realize that for the workflow of "push a branch, then immediately make a pull request" it's duplicate work. But that's not the only workflow. Another useful workflow is "push a branch, then look at the travis status to determine whether additional work is needed before making a push request." I think this workflow is useful too, and if the duplicate work in the former case isn't costing too much, I think it's worth putting up with it in order to allow the latter case.

How much is the duplicate work costing us, anyhow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's free. :)

Feel free to change it, no worries!

@devoncarew
Copy link
Contributor

Lgtm

If appveyor is still unhappy after this lands I can help diagnose in the PM.

sethladd added a commit that referenced this pull request Aug 17, 2015
@sethladd sethladd merged commit c5ab946 into master Aug 17, 2015
@sethladd sethladd deleted the appveyor branch August 17, 2015 23:36
@sethladd
Copy link
Contributor Author

Thanks. I'll watch the builds.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants