Skip to content

CI: use circleci #7603

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

Merged
merged 2 commits into from
Oct 28, 2020
Merged

CI: use circleci #7603

merged 2 commits into from
Oct 28, 2020

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Oct 26, 2020

Travis cut to half the number of builds for open source projects,
this is slower builds. We are already paying for circleci.

Circleci doesn't support "allowing failures", so I didn't add the docs-linkcheck job here.

@stsewd stsewd force-pushed the circleci branch 4 times, most recently from 612deab to 24b545e Compare October 26, 2020 23:31
@stsewd stsewd added the PR: work in progress Pull request is not ready for full review label Oct 26, 2020
@stsewd stsewd force-pushed the circleci branch 4 times, most recently from 9194052 to 8a796c7 Compare October 27, 2020 00:08
Travis cut to half the number of builds for open source projects,
this is slower builds. We are already paying for circleci.
Comment on lines 28 to 62
lint:
<<: *default
docker:
- image: 'cimg/python:3.6'
environment:
TOXENV: lint

migrations:
<<: *default
docker:
- image: 'cimg/python:3.6'
environment:
TOXENV: migrations

docs:
<<: *default
docker:
- image: 'cimg/python:3.6'
environment:
TOXENV: docs

docs-lint:
<<: *default
docker:
- image: 'cimg/python:3.6'
environment:
TOXENV: docs-lint

eslint:
<<: *default
docker:
- image: 'cimg/python:3.6'
environment:
TOXENV: eslint
NODE_VERSION: 10.17.0
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like we can speed up things by running all these in one job, otherwise circle will create a new container per each job and we can only have 5 concurrent jobs

Copy link
Member

@ericholscher ericholscher Oct 27, 2020

Choose a reason for hiding this comment

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

I don't fully follow. Meaning just have 1 job that calls tox and it runs all 5 steps? I think we do that on the ad server: https://github.com/rtfd/ethical-ad-server/blob/ede6a0a0d2f9609f9e6426f59b4666219038a882/.circleci/config.yml#L27

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, just call tox with all except for the py36 environment (since that requires ES running, but maybe isn't that bad having ES running during all tests?). Also, splitting will give us the linter failures more quickly :D

Copy link
Member

Choose a reason for hiding this comment

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

I think 1 job per tox step is easier to parse while reading the errors. Right now we have 6 check notifications in this PR (1 per job) and you know immediately what (docs, lint, etc) failed without going to CircleCI.

Also, if we have 1 job per tox step, don't they run in parallel?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, if we have 1 job per tox step, don't they run in parallel?

Yeah, but then circle will be creating one container per each task, and with our current plan we can only have 5 concurrent jobs. So other PRs will need to wait till there is space available.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can run tox -e <env> on separate steps so is more clear what failed

@stsewd stsewd removed the PR: work in progress Pull request is not ready for full review label Oct 27, 2020
@stsewd stsewd requested a review from a team October 27, 2020 00:14
@humitos
Copy link
Member

humitos commented Oct 27, 2020

by using CircleCI, will the output of the tests be public? we need a way to configure that probably, because right know I can't access without being logged in in CircleCI.

@stsewd
Copy link
Member Author

stsewd commented Oct 27, 2020

The project is already marked as open source. Looks like you need to be logged-in anyway...

Screenshot_2020-10-27 Advanced Settings - readthedocs org

@ericholscher
Copy link
Member

ericholscher commented Oct 27, 2020

I think 2 checks seems like a good compromise. 👍

Requiring login is annoying though. Do we know if all users who login can see it, or do they need repo access?

@stsewd
Copy link
Member Author

stsewd commented Oct 27, 2020

According to the docs they should be able without having repo access. But I don't have another account to test with :/

Copy link
Member

@humitos humitos 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! Having at least 2 jobs is good 💯

@humitos
Copy link
Member

humitos commented Oct 28, 2020

Circleci doesn't support "allowing failures", so I didn't add the docs-linkcheck job here.

What should we do with this check? Why do we need to allow this test to fail?

@ericholscher
Copy link
Member

What should we do with this check? Why do we need to allow this test to fail?

I think it's a "nice to have in the future" set of checks that we haven't enabled yet. I think we can probably just not run them, until they pass?

@ericholscher ericholscher merged commit b3c54a1 into master Oct 28, 2020
@ericholscher ericholscher deleted the circleci branch October 28, 2020 14:33
@stsewd
Copy link
Member Author

stsewd commented Oct 28, 2020

What should we do with this check? Why do we need to allow this test to fail?

I think is fine running the check locally, I never looked at that job. It fails because the rate limit from github (the changelog has a lot of links to github).

@humitos
Copy link
Member

humitos commented Oct 28, 2020

Sounds good! I created an issue (#7212) to re-enable this check in the future. Maybe it's possible to disable the check in some pages to avoid this rate limit.

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