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

Migrate CI to GH actions #4924

Merged
merged 41 commits into from Jan 1, 2021
Merged

Conversation

elacuesta
Copy link
Member

@elacuesta elacuesta commented Dec 11, 2020

Based on #4814

@elacuesta elacuesta added the CI label Dec 11, 2020
@codecov
Copy link

codecov bot commented Dec 16, 2020

Codecov Report

Merging #4924 (988d733) into master (39bc9a4) will decrease coverage by 0.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4924      +/-   ##
==========================================
- Coverage   88.01%   87.83%   -0.18%     
==========================================
  Files         158      158              
  Lines        9719     9719              
  Branches     1433     1433              
==========================================
- Hits         8554     8537      -17     
- Misses        910      928      +18     
+ Partials      255      254       -1     
Impacted Files Coverage Δ
scrapy/robotstxt.py 75.30% <0.00%> (-22.23%) ⬇️
scrapy/extensions/memusage.py 50.00% <0.00%> (+1.08%) ⬆️

tests/test_webclient.py Outdated Show resolved Hide resolved
@elacuesta elacuesta marked this pull request as ready for review December 16, 2020 22:44
@elacuesta elacuesta force-pushed the tests-github-actions branch 3 times, most recently from 443415e to 18bf452 Compare December 24, 2020 15:22
@elacuesta
Copy link
Member Author

elacuesta commented Dec 24, 2020

@Gallaecio @kmike @wRAR Uploading to the testing PyPI works: see https://github.com/elacuesta/scrapy/runs/1605715787 for 5521f2b (removed other jobs to save time, hardcoded name and version - different name since scrapy is taken). Available at https://test.pypi.org/project/Scrapy-Test/

@kmike
Copy link
Member

kmike commented Dec 24, 2020

Nice @elacuesta!

I meant to ask this before - do you know if wwe can have several workflow files, and e.g. separate test workflow from the publishing workflow?

@elacuesta
Copy link
Member Author

Indeed it's possible, I just split them into four workflows, looks like a good idea to improve organization. One missing thing though seems to be the ability to depend on jobs from other workflows (the needs key only works within the same workflow, as seen in this test flow). Initially I configured the jobs to run the publish one only if the previous ones succeeded, but perhaps we don't mind about that?


jobs:
tests-ubuntu:
name: Tests - Ubuntu
Copy link
Member

Choose a reason for hiding this comment

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

This is super-minor, but Github shows status as Tests - Ubuntu / Tests - Ubuntu

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, seems hard to change though. If the workflow name is omitted, the full name of the file is used, while if the job name is omitted, the job key is used:
no_names

I tried setting the same name for the two test files, hoping they would get grouped, but it wasn't the case:
Same name - not grouped

Copy link
Member

@kmike kmike Dec 30, 2020

Choose a reason for hiding this comment

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

Again, a nitpick, but what do you think about using shorter names? This is how the popup looks like, when you click on a check mark near a commit:

изображение

Proposal: instead of naming a job "tests-ubuntu", name it "tox"; the same for windows. Keep workflow names as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, names are still too long, thanks for insisting on this. I just renamed both the workflows and the jobs, looks much cleaner to me, what do you think?
ci-popup

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

Looks good to me. 🤞 about publish.yml.

README.rst Outdated Show resolved Hide resolved
@kmike kmike mentioned this pull request Dec 30, 2020
@Gallaecio
Copy link
Member

Gallaecio commented Dec 31, 2020

Given there’s 1 badge per workflow, I wonder if maybe it would make sense to keep only 2 workflows, tests and publish. But badges are not really a strong argument, so do not take it to heart, just a thought :)

Copy link
Member

@kmike kmike left a comment

Choose a reason for hiding this comment

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

👍

@elacuesta elacuesta merged commit 80db569 into scrapy:master Jan 1, 2021
@elacuesta elacuesta deleted the tests-github-actions branch February 9, 2021 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants