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 #7564

Merged
merged 2 commits into from
Jan 7, 2020
Merged

Remove AppVeyor #7564

merged 2 commits into from
Jan 7, 2020

Conversation

chrahunt
Copy link
Member

@chrahunt chrahunt commented Jan 7, 2020

With Azure Pipelines we get 30 parallel jobs. Currently for a single PR we're using a maximum of 7 concurrently at a time. In practice it will be less because Linux and macOS finish pretty quickly.

With AppVeyor we get 1 parallel job between pip and setuptools.

By switching all of our AppVeyor tests to Azure Pipelines, this brings our maximum concurrent jobs per PR to 9 on Azure Pipelines and 0 on AppVeyor. This means that we can have CI progressing on a minimum of 3 PRs at a time vs previously all PRs were queuing on AppVeyor.

Not to mention we now only need to focus on 3 CI providers (Travis, Azure Pipelines, GitHub) instead of 4.

This will allow us to have CI running concurrently on multiple PRs,
since we get 30 parallel jobs on Azure Pipelines but only 1 on AppVeyor.

We have parameterized --use-venv since AppVeyor was using it, but Azure
Pipelines was previously not.
@chrahunt chrahunt added skip news Does not need a NEWS file entry (eg: trivial changes) C: automation Automated checks, CI etc type: maintenance Related to Development and Maintenance Processes labels Jan 7, 2020
@pradyunsg
Copy link
Member

I like this idea -- it makes sense to drop AppVeyor.

The small number of AppVeyor jobs is most evident when multiple PRs are made in a short period of time, and hampers the workflow when folks are most active.

@chrahunt
Copy link
Member Author

chrahunt commented Jan 7, 2020

How do we want to do this? Disable the required AppVeyor check and be mindful of any existing PRs? I think we have few enough now that it is not a big risk.

@pradyunsg
Copy link
Member

Sounds about right to me. :)

@xavfernandez
Copy link
Member

xavfernandez commented Jan 7, 2020

I'm a little torn since Appveyor seems to have a slightly different setup than Azure (I think we've already had some PR passing on Azure but not Appveyor or the opposite) which might catch (or cause ;) ) some issues that would go undetected on Azure.
And in my opinion every additionnal job is good to have.

But I also completely agree that Appveyor is often the bottleneck of our CI when multiple PR are updated concurrently, we could maybe reduce the Appveyor load to a single CPython version ?

PS: But apparently since I worked around the discrepancy between Azure & Appveyor my argument is kinda moot so I'll follow your decision ^^

@chrahunt
Copy link
Member Author

chrahunt commented Jan 7, 2020

If we have any regret it is straightforward to revert back, but I highly suspect that we won't. :)

@chrahunt chrahunt marked this pull request as ready for review January 7, 2020 13:15
@chrahunt
Copy link
Member Author

chrahunt commented Jan 7, 2020

I unrequired the check, and should be able to disable it once this is merged.

@pradyunsg pradyunsg merged commit 9e910aa into pypa:master Jan 7, 2020
@pradyunsg
Copy link
Member

Whoooops. I missed @xavfernandez's comment above (it wasn't visible when I opened this issue from the notification, since I'd been scrolled down automatically).

@xavfernandez
Copy link
Member

No issue, let's call it fate ;)
And like Chris said it's unlikely we'll regret this bottleneck :)

@pradyunsg
Copy link
Member

I just went ahead and dropped the push event from our AppVeyor webhook -- that should result in a green tick for master when we merge our next PR.

@chrahunt chrahunt deleted the maint/remove-appveyor branch January 7, 2020 22:10
@chrahunt
Copy link
Member Author

chrahunt commented Jan 7, 2020

I unchecked "active" for the webhook, since otherwise it was still running (and failing) on PRs, e.g. on push to #7354 after release. Feel free to change it back if there is a more graceful way to handle it that we feel like investing in.

@xavfernandez
Copy link
Member

We might want to completely delete the webhook & uninstall the github app (if it does not impact setuptols) in a near future.

@hugovk hugovk mentioned this pull request Jan 13, 2020
5 tasks
@pradyunsg pradyunsg mentioned this pull request Jan 28, 2020
12 tasks
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Feb 6, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: automation Automated checks, CI etc skip news Does not need a NEWS file entry (eg: trivial changes) type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants