Skip to content

Add appveyor.yml config file#299

Merged
nfischer merged 1 commit intoshelljs:masterfrom
nfischer:appveyor-support
Jan 27, 2016
Merged

Add appveyor.yml config file#299
nfischer merged 1 commit intoshelljs:masterfrom
nfischer:appveyor-support

Conversation

@nfischer
Copy link
Copy Markdown
Member

This is a redo of #298

@nfischer
Copy link
Copy Markdown
Member Author

I will close this if appveyor doesn't get triggered

@ariporad
Copy link
Copy Markdown
Contributor

LGTM, but let's wait to be able to actually enable AppVeyor.

@nfischer
Copy link
Copy Markdown
Member Author

@ariporad yup. I think this shouldn't be merged until appveyor is set up properly and it actually runs on the PR (so we might have to reopen another PR after that and close this one).

@mtscout6
Copy link
Copy Markdown

You can always rebase this change on the latest from master then force push the branch. That will re-trigger AppVeyor just fine without need to close/re-open this PR. If the branch is up to date with the latest from master you can amend this commit (change the commit message) and force push which will also re-trigger things in AppVeyor. Once the Webhook stuff is triggered that is.

@nfischer
Copy link
Copy Markdown
Member Author

Yeah, that's one way. I also wanted to verify that it works on PRs as well. But since the config file is most likely good as it is, we can probably just merge once the appveyor settings and webhooks are straightened out.

@ariporad
Copy link
Copy Markdown
Contributor

@nfischer: I think we might want to get our tests passing on windows before this gets merged. But it did work!

@nfischer
Copy link
Copy Markdown
Member Author

@ariporad sounds good to me

@mtscout6
Copy link
Copy Markdown

@ariporad You say it worked, but I don't see an AppVeyor status check on this PR for AppVeyor.

@nfischer
Copy link
Copy Markdown
Member Author

@mtscout6 appveyor should now be running on this PR (I updated an equivalent PR, but forgot to do this one)

@mtscout6
Copy link
Copy Markdown

Perfect! Glad to see it working!

@nfischer
Copy link
Copy Markdown
Member Author

Yup! And it looks similar to the output @BYK got when running the tests, which is a good sign

@nfischer nfischer mentioned this pull request Jan 27, 2016
@mtscout6
Copy link
Copy Markdown

@ariporad I'd advocate merging this now instead of waiting for tests to pass. That way the you can see that the tests do fix the tests in the PRs that are fixing them. It's not a "Required" check for merging, though once it goes green then you can make it a required check if you like. That's my opinion though.

@ariporad
Copy link
Copy Markdown
Contributor

@mtscout6: hmm... Ok. That makes sense. @nfischer: FFTM (Feel Free To Merge).

@nfischer
Copy link
Copy Markdown
Member Author

LGTM

nfischer added a commit that referenced this pull request Jan 27, 2016
@nfischer nfischer merged commit 7d6249a into shelljs:master Jan 27, 2016
@nfischer nfischer mentioned this pull request Jan 28, 2016
@nfischer nfischer deleted the appveyor-support branch January 28, 2016 07:02
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.

3 participants