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

Run check-manifest and other linters via GitHub Actions. #396

Merged
merged 12 commits into from
Aug 17, 2022

Conversation

adiroiban
Copy link
Member

@adiroiban adiroiban commented Jul 16, 2022

Description

This is take2 for #292

Enabled check manifest and pre-comit as part of GitHub Actions CI

We have pre-commit hook so I am not sure if this is needed.

I tried to simplify the job and removed the job matrix.

Fixes #292

Checklist

  • [NA] Make sure changes are covered by existing or new tests.
  • [NA] For at least one Python version, make sure local test run is green.
  • Create a file in src/towncrier/newsfragments/. Describe your
    change and include important information. Your change will be included in the public release notes.
  • Make sure all GitHub Actions checks are green (they are automatically checking all of the above).

@adiroiban adiroiban changed the title 292 more ci checks Run check-manifest and other linters via GitHub Actions. Jul 16, 2022
Copy link
Member

@altendky altendky left a comment

Choose a reason for hiding this comment

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

Not my personal CI style (#292 (comment)) but it'll work. Here's a few thoughts.

CONTRIBUTING.rst Outdated Show resolved Hide resolved
@@ -33,37 +33,4 @@ commands =
# specifying the entry point script in `{envbindir}`.
coverage run -p --module twisted.trial {posargs:towncrier}
coverage combine -a

[testenv:build]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this going away?

Copy link
Member Author

Choose a reason for hiding this comment

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

my bad. I thought that we don't need it and that it was already moved to GitHub CI only.

I will revert...

My hope was that we can use standard tools without the need of extra tools.

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure where you were going so maybe it isn't needed now? It was an honest question.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is still need.
The towncrier is built once.
Then the wheel is uploaded as an artifact.
Then the tests will download the artifact and run the tests agains it.


I am not convinced that we need all this dance... but I think is better to refactor this in another PR.

@@ -0,0 +1,3 @@
[flake8]
# Allow for longer test strings. Code is formatted to 88 columns by Black.
max-line-length = 99
Copy link
Member

Choose a reason for hiding this comment

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

I'm still in the boat of using the program's default config file. Even when using the 'single config file for everything' option you still end up with multiple single config files.

Copy link
Member Author

Choose a reason for hiding this comment

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

The plan it to move more things from setup.py into setup.cfg ... I am using it in #398

Copy link
Member

Choose a reason for hiding this comment

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

Any project using black will still have to have a pyproject.toml for any config there, afaik. Anyways, not a bug, just a passing opinion.

Comment on lines 175 to 182
- name: Tox
run: tox -c tox.ini -e ${{ matrix.task.tox }}
run: tox -c tox.ini --parallel -e pre-commit

- name: Tox
run: tox -c tox.ini --parallel -e check-newsfragment

- name: Tox
run: tox -c tox.ini --parallel -e check-manifest
Copy link
Member

Choose a reason for hiding this comment

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

Use the relevant names for the step name. Why parallel for single environments? How about if: always() so you get all results in a single run rather than having to iterate through multiple failures via multiple commits and ci runs.

uses: actions/setup-python@v3
with:
python-version: ${{ matrix.python.action }}
python-version: "3.10"
Copy link
Member

Choose a reason for hiding this comment

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

These days I tend to keep my checks one version behind (3.9 not 3.10). They are there to check the code, not see if the tools work with the latest Python. But definitely not a big deal, just sharing a thought.

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 will revert. thanks. no big preference.
Just trying to reduce the number of redirections :)

@codecov
Copy link

codecov bot commented Jul 17, 2022

Codecov Report

Merging #396 (1bb6221) into trunk (07baa2b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            trunk     #396   +/-   ##
=======================================
  Coverage   97.85%   97.85%           
=======================================
  Files          22       22           
  Lines        1400     1400           
  Branches      130      130           
=======================================
  Hits         1370     1370           
  Misses         15       15           
  Partials       15       15           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@adiroiban
Copy link
Member Author

Thanks for your help.

In the end, this is not much better.

it just helped me understand the (what I consider convoluted) way in which towncrier CI works... with all the build phase and tox_build.sh external script.

I would still like to merge it as I hope that in the future we can update the CI and remove the build and tox_build.sh script.

@adiroiban adiroiban enabled auto-merge August 17, 2022 17:28
@adiroiban
Copy link
Member Author

I have enbabled auto-merge for this.

There are some README updates...and no other big changes.

@adiroiban adiroiban merged commit 5bf0a46 into trunk Aug 17, 2022
@adiroiban adiroiban deleted the 292-more-ci-checks branch August 17, 2022 17:30
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.

2 participants