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

Remove AppVeyor #1975

Merged
merged 3 commits into from
Dec 12, 2021
Merged

Remove AppVeyor #1975

merged 3 commits into from
Dec 12, 2021

Conversation

fearphage
Copy link
Contributor

@fearphage fearphage commented Dec 11, 2021

Most of the ReadMe changes are dead white space at the end of lines.

View without white space changes: https://github.com/winstonjs/winston/pull/1975/files?diff=unified&w=1

We need someone to disable AppVeyor so it doesn't erroneously fail builds.

@fearphage fearphage requested review from DABH and wbt December 11, 2021 19:29
Copy link
Contributor

@DABH DABH left a comment

Choose a reason for hiding this comment

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

One nit but otherwise good!

README.md Outdated
[![Version npm](https://img.shields.io/npm/v/winston.svg?style=flat-square)](https://www.npmjs.com/package/winston)
[![npm Downloads](https://img.shields.io/npm/dm/winston.svg?style=flat-square)](https://npmcharts.com/compare/winston?minimal=true)
[![build status](https://github.com/winstonjs/winston/actions/workflows/ci.yml/badge.svg)](https://github.com/winstonjs/winston/actions/workflows/ci.yml)
[![Dependencies](https://img.shields.io/david/winstonjs/winston.svg?logo=npm&style=flat-square)](https://david-dm.org/winstonjs/winston)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the David badge 🙏

@DABH
Copy link
Contributor

DABH commented Dec 11, 2021

Do I need to disable Appveyor in the repo settings or from the Appveyor site or? Let me know and I’ll give it a try (I think it’s my fault it’s there in the first place).

That being said — The reason we added that in the first place was so that we could run the tests on windows, which sometimes behaved a little different than on Linux. Does GitHub actions support testing on multiple platforms? Should we still care, or hope that the ecosystem has matured enough that platform discrepancies shouldn’t be a concern?

@fearphage
Copy link
Contributor Author

Do I need to disable Appveyor in the repo settings or from the Appveyor site or?

Yes, you need to tell AppVeyor to ignore this project so it stop it marking builds as failed. The last one failed because the appveyor.yml doesn't exist which is the desired state.

@DABH
Copy link
Contributor

DABH commented Dec 11, 2021

@fearphage I think I have deleted the project from AppVeyor, hopefully we're good now?

@fearphage fearphage closed this Dec 12, 2021
@fearphage fearphage reopened this Dec 12, 2021
@fearphage fearphage merged commit 1a3ff33 into winstonjs:master Dec 12, 2021
@fearphage fearphage deleted the remove-appveyor branch December 12, 2021 16:01
@fearphage
Copy link
Contributor Author

fearphage commented Dec 12, 2021

@DABH Looks like it didn't work. I merged to master and still see AppVeyor:

image

We should stop merging PRs until we get this fixed. They will all fail now.

I believe you need to delete the project in AppVeyor. I'm guessing based on this ticket. stevengj/nlopt#421

@DABH
Copy link
Contributor

DABH commented Dec 12, 2021

Hmm I thought I did but might need to email @indexzero , I’ll do that…

@wbt
Copy link
Contributor

wbt commented Dec 14, 2021

This is reported done by @indexzero.

@fearphage
Copy link
Contributor Author

Confirmed in several PRs (#1986) and on the main branch.

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