Page MenuHomePhabricator

Set up easily reused CI settings
Open, Needs TriagePublic4 Estimated Story Points

Description

With more developers working in more projects it would be nice to have some easily deployable CI settings/rules/files.

In a first pass these would probably be (for python on Github):

  • A basic tox.ini file doing flake8 and pydocstyle linting (and running any tests) for python 3.
  • .travis.yml for automatic
  • Instructions for setting up webhooks to make the above run on (at least) every pull request.
  • Merge demo into COH-tools
  • Documentation and links on the Development Guidelines page.
  • Stick basic setting files in an stand-alone location/repo.

Outcome

Basic settings have been done for COH-tools in /lokal-profil/COH-tools/tree/CI.

Files:

  • tox.ini: Main entrypoint specifying python version to use, and tests and linting to run.
  • setup.cfg: Specific settings for nosetests mostly to do with coverage.
  • .travis.yml: The connection needed for TravisCI to run the tox file.
  • requirements.txt: The dependencies needed to run the program. Install these using pip3 install -r requirements.txt (possibly with the --process-dependency-links flag).
  • requirements-test.txt: The additional dependencies needed to run the tests.
Notes on files:
  • [tox.ini] I've set pydocstyle to ignore D100-D103 errors (missing docstrings). These shouldn't be blockers for now but docstrings should be added whenever a function is updated/added. The long term goal is to not ignore these errors and local linting might wish to have these enabled. As adding your own ignores block the default ones I've also had to add D203 and D212.
  • [tox.ini] I've set flake8 to ignore E501 errors (too long lines). These shouldn't be blockers for now but strings should be fixed whenever a function is updated/added. The long term goal is to not ignore these errors and local linting might wish to have these enabled. For individual exceptions (such as long urls) use the following comment at the end of the line: # noqa: E501.
  • [setup.cfg] Without a proper package setup the output isn't ideal. Might consider adding cover-no-print when packages are not available and cover-inclusive when they are?
  • [requirements.txt] We include mwparserfromhell here since pywikibot silently changes it behaviour (for the worse) if it is not available.
  • [requirements.txt] Note that we connect to the master branch of pywikibot (not the PyPi release) and to a specific release of WikidataStuff.
  • [requirements-test.txt] The first line (-r requirements.txt) ensures that the normal dependencies are also installed.

Re-use of files:

  • [tox.ini] The file can be largely reused as is with the exception for [flake8] β†’ filename, [pydocstyle] β†’ match-dir and any unneeded ignores.
  • [setup.cfg] Can be re-used as is with the exception of cover-package
  • [.travis.yml] Can be re-used as is.
  • [requirements.txt] Will need to be rebuilt for each repo.
  • [requirements-test.txt] Can be re-used as is (but may need to be extended at a later stage).

This assumes a python3 only repo. If you also want it to be compatible with python27 a few files need to be further tweaked.

Connections:

I make connections to two services which will later vote on any PRs (and commits) allowing me to do some initial improvements before passing the PR on for review.

  • I added the repo to my travis-ci.org account and enabled the "Build only if .travis.yml is present" setting. Since my TravisCI account is hooked up to my Github account it automatically added the hook to Settings β†’ Integrations & services β†’ Services (in GitHub).
  • I also added the repo to my quantifiedcode.com account. Again since it's connected to my Github account it automatically added the hook to Settings β†’ Webhooks (in GitHub). I normally rely on quantifiedcode to warn me about undesired (but not strictly wrong) behaviour. It is also useful to ensure you don't introduce new problems (such as missing docstrings) in a PR. It's detection algoritm isn't perfect though so I tend to use it as a recommendation rather than a blocker.

Notes on connections:

  • [TravisCI] The first couple of runs will allow you to identify if any dependencies are missing from requirements.txt (in the tox_env=py3 log)
  • [quantifiedcode] You can adjust the settings further by going to Settings β†’ Webhooks and clicking the quanified code link. Specifically you can make it just trigger on PRs etc.

Event Timeline

@Sebastian_Berlin-WMSE @Alicia_Fagerving_WMSE Would you mind taking a look at the "Result" section above and give a shout about bits which are unclear/missing/unnecessary? Please add these as comments (typos/formatting you may of course edit directly). The plan is to, when ready, add this as a subpage to the DevGuidelines.

I guess there are some additional assumptions I've made (such as tests being in /tests/ directly under root). If you find any such assumptions it might be good to drop a note.

Re: pip install -r requirements.txt -- if it's a python3-only repo, would it make sense to use pip3 install?

Re: pip install -r requirements.txt -- if it's a python3-only repo, would it make sense to use pip3 install?

Good point.
How clear/confusing is this overall?

Re: pip install -r requirements.txt -- if it's a python3-only repo, would it make sense to use pip3 install?

Good point.
How clear/confusing is this overall?

I do think I understand what the different parts are doing -- I guess if there's any problems they'll crop up when actually trying to set it all up, but I assume since it's working for you then it shouldn't just break here.

I haven't used most of the tools here, so I can't give much feedback on how they're used. Here are a few thoughts on the description:

  • [tox.ini] I've set flake8 to ignore E501 errors (too long lines). These shouldn't be blockers for now but strings should be fixed whenever a function is updated/added. The long term goal is to not ignore these errors and local linting might wish to have these enabled.

In my experience, this is one of those rules that you almost always want to follow, but there are exceptions. E.g. when you need to have a long URL or HTML string, I find it's easier to read if it's in a continuous string, rather than to split it to follow max line length. A few cases of this can be allowed if there is a way to ignore them (e.g. by adding a comment).

  • [requirements-test.txt] I've included mock here even though it is not (yet) needed for COH-tools.

Is this a different mock than https://docs.python.org/3/library/unittest.mock.html?

Since my TravisCI account is hooked up to my Github account it automatically added the hook to Settings β†’ Integrations & services β†’ Services.

and

Again since it's connected to my Github account it automatically added the hook to Settings β†’ Webhooks.

Are these settings in Github or the respective tools?

Are these settings in Github or the respective tools?

These are Github settings, though I'm not sure about the technical difference between 'webhooks' and 'services'.... In any case, you can log in to Travis / Quantified Code with your Github account, and then select the project to synchronize. Then if you go back to Github, the connections should have been created automatically, QC under 'webhooks' and Travis under 'Integrations and services'.

Re: pip install -r requirements.txt -- if it's a python3-only repo, would it make sense to use pip3 install?

Good point.
How clear/confusing is this overall?

I do think I understand what the different parts are doing -- I guess if there's any problems they'll crop up when actually trying to set it all up, but I assume since it's working for you then it shouldn't just break here.

If there are specific parts then ask here. If nothing else we can add it to the description/documentation.

  • [tox.ini] I've set flake8 to ignore E501 errors (too long lines). These shouldn't be blockers for now but strings should be fixed whenever a function is updated/added. The long term goal is to not ignore these errors and local linting might wish to have these enabled.

In my experience, this is one of those rules that you almost always want to follow, but there are exceptions. E.g. when you need to have a long URL or HTML string, I find it's easier to read if it's in a continuous string, rather than to split it to follow max line length. A few cases of this can be allowed if there is a way to ignore them (e.g. by adding a comment).

While I agree (and it might be worth mentioning the "skip this line" comment) the main reason this is deactivated now is because I wouldn't consider pre-existing long lines as blockers for an unrelated new patch. Of course no new long lines should be introduced and the plan would be to eliminate them all and reactivate that test.

  • [requirements-test.txt] I've included mock here even though it is not (yet) needed for COH-tools.

Is this a different mock than https://docs.python.org/3/library/unittest.mock.html?

I believe this might be a py27 -> py3 thing. Was mock maybe not part of the unittest library in py27 but is now?

Are these settings in Github or the respective tools?

These are Github settings, though I'm not sure about the technical difference between 'webhooks' and 'services'.... In any case, you can log in to Travis / Quantified Code with your Github account, and then select the project to synchronize. Then if you go back to Github, the connections should have been created automatically, QC under 'webhooks' and Travis under 'Integrations and services'.

I believe webhooks simply interact via the API whereas services might be more closely integrated. From github-services I would also guess that this is legacy and largely replaced by the webhooks.

Would it be simpler if I moved setup.cfg into tox.ini?

Added clarification that Settings are in GitHub and a comment on noqa to excempt individual lines.

Would it be simpler if I moved setup.cfg into tox.ini?

If setup.cfg is only relevant for the tests, then it makes sense to keep it separate.

  • [requirements-test.txt] I've included mock here even though it is not (yet) needed for COH-tools.

Is this a different mock than https://docs.python.org/3/library/unittest.mock.html?

I believe this might be a py27 -> py3 thing. Was mock maybe not part of the unittest library in py27 but is now?

I've dropped this from the requirements-test.txt

I cleaned up the documentation some more and added it to wmse:Development_guidelines/CI_for_python

The current ignore settings allow for some contradictory rules. I have added D203, D212 and D404 to my ignore list.

The current ignore settings allow for some contradictory rules. I have added D203, D212 and D404 to my ignore list.

Thanks. I'll add D203, D212 to the demo tox and the documentation(s). Leaving D404 because why not.

This was re-discovered as part of the recent dev guidelines update. We should go through it and make sure its up-to-date, move the files to a dedicated repo then embedd it in the guidelines.

Also ade codecov and step-by-step guide to prepping a repo

On my todo but sadly very low priority for now

If we want to outsource our opinions we can use Black for code style. I tested running it on the project start bot and that didn't change that much: https://github.com/Wikimedia-Sverige/project-start/commit/3380fb9d9bc97cb570c9ec3023b2aadbb0a3e690.

Personally, I think the long lines (up 88 characters) is the biggest change, but I can probably learn to live with that.

Now that there is a Gitlab for Wikimedia we may want to have a look at what the CI setup is like over there. If we want to move over, that is.