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

Update CI to test on all major OS and remove Appveyor #1757

Merged
merged 4 commits into from
Feb 9, 2022

Conversation

tlylt
Copy link
Contributor

@tlylt tlylt commented Feb 9, 2022

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Overview of changes:
Related to: #1386

  • Remove redundant Appveyor script
  • Include macOS and Windows in CI test
  • Update setup-node to v2

Anything you'd like to highlight / discuss:
Not sure if I have the rights to turn off Appveyor, might need one of the senior devs to help with this.
(Update: will indeed need someone with the access :)
Testing instructions:
nil

Proposed commit message: (wrap lines at 72 characters)
Improve CI script

Currently, we are using GitHub actions and Appveyor to
run tests for PRs.

Let's remove the use of Appveyor and consolidate CI tests
in GitHub actions. Let's also update the setup-node action
version.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@tlylt tlylt self-assigned this Feb 9, 2022
Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Thanks @tlylt!

LGTM 👍

Not sure if I have the rights to turn off Appveyor, might need one of the senior devs to help with this.
(Update: will indeed need someone with the access :)

Seems like I don't have the permission as well to uninstall the Appveyor app. Perhaps @ang-zeyu can take a look at this?

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@tlylt tlylt changed the title Update CI to test on all major OS and include caching Update CI to test on all major OS Feb 9, 2022
@tlylt tlylt changed the title Update CI to test on all major OS Update CI to test on all major OS and remove Appveyor Feb 9, 2022
@ang-zeyu ang-zeyu added this to the 4.0 milestone Feb 9, 2022
@ang-zeyu ang-zeyu merged commit 1a98254 into MarkBind:master Feb 9, 2022
@ryoarmanda
Copy link
Contributor

Hi guys, I just want to drop in about this merge. It seems that currently open PRs still runs the old CI workflow (ci-test instead of test). Is there any way for their PRs to switch to this new workflow without creating a new PR?

I might try closing and reopening a PR, to see whether the reopening would trigger the new workflow instead.

@tlylt
Copy link
Contributor Author

tlylt commented Feb 10, 2022

Hi guys, I just want to drop in about this merge. It seems that currently open PRs still runs the old CI workflow (ci-test instead of test). Is there any way for their PRs to switch to this new workflow without creating a new PR?

I might try closing and reopening a PR, to see whether the reopening would trigger the new workflow instead.

Would merging the master back to the PR branch work? I am thinking because the PR branch may not be up to date that's why the CI is a bit weird...

@ryoarmanda
Copy link
Contributor

ryoarmanda commented Feb 10, 2022

Update: closing and reopening PR works in triggering the new workflow. However, the required checks are still of the old workflows. Plus Travis (edit: Appveyor, brain fart there) is still there, but as we are not using it anymore, it immediately throws a failing check.

Let me see if I can modify the status checks for PRs to master

@tlylt
Copy link
Contributor Author

tlylt commented Feb 10, 2022

Update: closing and reopening PR works in triggering the new workflow. However, the required checks are still of the old workflows. Plus Travis is still there, but as we are not using it anymore, it immediately throws a failing check.

Let me see if I can modify the status checks for PRs to master

Are you referring to Appveyor? Once Ze Yu disabled it on the Appveyor website I think it should go away. For the Travis we are currently using it to deploy docs (if I am not wrong)

P.S I am looking to work on migrating the Travis script over to Github action over the weekend :)
image

@ryoarmanda
Copy link
Contributor

ryoarmanda commented Feb 10, 2022

Yeah, just had a brain fart there haha, I mean the Appveyor one. The build script seems to be gone already, but for some reason it is still triggered.

image

@ryoarmanda
Copy link
Contributor

ryoarmanda commented Feb 10, 2022

Okay, I have changed the required status checks to master to the new workflow (already reflected on @tlylt 's screenshot).

Also, I just noticed that we still have an active webhook to Appveyor on PRs and pushes. I deactivated it (but still exists) just to try out whether Appveyor would stop triggering builds and show up on the status check.

(Update: recently merged PR does not trigger the build, yay?)

@ang-zeyu @jonahtanjz is there anything else that we need to do to accommodate the new workflow?

@tlylt
Copy link
Contributor Author

tlylt commented Feb 10, 2022

Also, I just noticed that we still have an active webhook to Appveyor on PRs and pushes. I deactivated it (but still exists) just to try out whether Appveyor would stop triggering builds and show up on the status check.

For the active webhook, are you referring to some repo settings or? I made the deletion of the Appveyor config file but I didn't notice any "webhook" or related settings in other files.

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