CI cleanup #450

wsanchez · 2021-02-10T00:16:45Z

No description provided.

adiroiban

Some tests were cancelled... not sure why.

I guess that this is due to this error - which is due to fail-fast.

https://github.com/twisted/klein/pull/450/checks?check_run_id=1867537890#step:6:7

My suggestion is to use shorter names so that you don't have to check the HTML source code in order to see the full error message.

In this case, "Cancelled after 5 min" is not fully visible

adiroiban · 2021-02-10T02:24:39Z

.github/workflows/cicd.yml

@@ -183,7 +183,7 @@ jobs:
    strategy:
      matrix:
        python: ["3.6", "3.7", "3.8", "3.9", "pypy3"]
-        twisted: ["19.2", "current"]
+        twisted: ["19.2", "current", "trunk"]


Maybe disable early failing that is causing cancellation and skipping of remaining jobs from the matrix.

Suggested change

twisted: ["19.2", "current", "trunk"]

twisted: ["19.2", "current", "trunk"]

fail-fast: false

wsanchez · 2021-02-10T03:17:15Z

Some tests were cancelled... not sure why.

I guess that this is due to this error - which is due to fail-fast.

https://github.com/twisted/klein/pull/450/checks?check_run_id=1867537890#step:6:7

My suggestion is to use shorter names so that you don't have to check the HTML source code in order to see the full error message.

In this case, "Cancelled after 5 min" is not fully visible

Yup sorry @adiroiban this isn't ready yet, I'll mark it as draft.

codecov · 2021-02-16T18:04:53Z

Codecov Report

Merging #450 (3e22097) into master (ce42479) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #450   +/-   ##
=======================================
  Coverage   98.71%   98.71%           
=======================================
  Files          45       45           
  Lines        3881     3881           
  Branches      249      249           
=======================================
  Hits         3831     3831           
  Misses         33       33           
  Partials       17       17

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce42479...3e22097. Read the comment docs.

wsanchez · 2021-02-16T21:48:31Z

.github/workflows/cicd.yml

+    runs-on: ${{ matrix.os }}
    timeout-minutes: 30
    strategy:
      matrix:
+        os: ["ubuntu-latest"]


To simplify adding OSes to the matrix later.

wsanchez · 2021-02-16T21:48:49Z

.github/workflows/cicd.yml

+        with:
+          fetch-depth: 2


This is needed by the Codecov action.

Yes. That should be ok.
For PR builds, you can use the extra depth to get the last commit message... as HEAD is a merge with the PR target branch.

For paid runs I use this to skip a build on macos for example... when I am working on fixing a windows only bug.

- name: Fail on "skip macos" if: ${{ github.event.after }} run: git log -1 --pretty=format:"%s" ${{ toJSON(github.event.after) }} | grep -v 'skip macos'

Not an issue with klein as GitHub Action is free )

wsanchez · 2021-02-16T21:50:51Z

.github/workflows/cicd.yml

+      - name: Translate Python version to Tox environment
+        id: tox_env
+        shell: python
        run: |
-          set -eux
-          py="${{ matrix.python }}";
-          if [ "${py#pypy}" != "${py}" ]; then  # PyPy
-              py_test="${py}";
-          else  # CPython
-              py_test="py${py/./}";  # Add "py" prefix, remove "."
-          fi;
-          twisted="${{ matrix.twisted }}";
-          twisted="tw${twisted/./}";  # Add "tw" prefix, remove "."
-          env_test="coverage-${py_test}-${twisted}";
-          echo "Test environment: ${env_test}";
-          ln -s ".tox/${env_test}" testenv;  # Fixed location for upload step below
-          tox -e "${env_test}";
+          py = "${{ matrix.python }}".replace(".", "").replace("pypy", "py")
+          tw = "${{ matrix.twisted }}".replace(".", "")
+          print(f"::set-output name=value::coverage-py{py}-tw{tw}")
+
+      - name: Run unit tests
+        run: tox -e ${{ steps.tox_env.outputs.value }};

      - name: Upload Trial log artifact
        if: failure()
        uses: actions/upload-artifact@v1
        with:
          name: trial
-          path: testenv/log/trial.log
+          path: .tox/${{ steps.tox_env.outputs.value }}/log/trial.log


This borrows a nice technique from Treq to map the Top environment in a step, then reuse the value later.

(Hat tip to @twm)

wsanchez · 2021-02-16T21:51:43Z

README.rst

+.. image:: https://github.com/twisted/klein/workflows/CI%2fCD/badge.svg
+    :target: https://github.com/twisted/klein/actions


Whoops no more Travis, yo

wsanchez · 2021-02-16T21:52:03Z

tox.ini

@@ -22,7 +22,7 @@ deps =
    tw1910: Twisted==19.10.0
    tw203: Twisted==20.3.0
    twcurrent: Twisted
-    twtrunk: git+https://github.com/twisted/twisted@trunk#egg=Twisted
+    twtrunk: https://github.com/twisted/twisted/tarball/trunk#egg=Twisted


Word is this is faster

wsanchez · 2021-02-16T21:53:00Z

tox.ini

+deps = pipdeptree

 commands =
-    pip freeze --exclude={env:PY_MODULE}
+    python -c 'print()'
+    pip freeze --exclude="{env:PY_MODULE}"
+
+    python -c 'print()'
+    pipdeptree


This prints out the pip freeze output, which I use to compare against what is in setup.py occasionally, and it now adds a dependency tree which is handy for seeing why certain things are showing up.

I am happy to see pipdeptree here :)

wsanchez · 2021-02-17T00:54:37Z

.github/workflows/cicd.yml

    timeout-minutes: 30
+    continue-on-error: ${{ matrix.optional }}


@adiroiban I found the continue-on-error docs have an example showing how to have a job not stop the workflow if it fails, so used that for trunk, which leaves fail-fast as something that we can turn on or off independently.

wsanchez · 2021-02-17T00:58:31Z

New feature: builds against Twisted trunk can now fail without failing the workflow, so you see "some checks were not successful" above, but the non-trunk jobs completed successfully, and the trunk job is not required, so it won't block merging.

But we do see an obvious Thing Of Interest that probably needs fixing.

wsanchez · 2021-02-17T00:58:55Z

I have as a final item to rename the unit test jobs so that they are more readable in the UI. I'll do that post-review, because it will require an associated config changes to keep the builds passing.

wsanchez · 2021-02-17T03:46:01Z

Unfortunately, this doesn't show up as "success" in the PR list, though it ideally should…

adiroiban

it looks good.

For the record.
There is this https://github.com/twisted/python-info-action which tries to share things like "Workflow information" ... maybe we can have "Workflow information" inside that action and use the action in klein and other Twisted projects.

As part of this PR, a maybe also enable Read The Docs pull builds.
Let me know if you need help wit that ... or ask Thomas over IRC for help.

Thanks!

adiroiban · 2021-02-17T22:32:28Z

.github/workflows/cicd.yml

+        with:
+          fetch-depth: 2


Yes. That should be ok.
For PR builds, you can use the extra depth to get the last commit message... as HEAD is a merge with the PR target branch.

For paid runs I use this to skip a build on macos for example... when I am working on fixing a windows only bug.

- name: Fail on "skip macos" if: ${{ github.event.after }} run: git log -1 --pretty=format:"%s" ${{ toJSON(github.event.after) }} | grep -v 'skip macos'

Not an issue with klein as GitHub Action is free )

adiroiban · 2021-02-17T22:35:25Z

.github/workflows/cicd.yml

-        # specified above to the equivalents used in Tox environments.
+      - name: Translate Python version to Tox environment
+        id: tox_env
+        shell: python


Nice move :)

adiroiban · 2021-02-17T22:40:15Z

tox.ini

+deps = pipdeptree

 commands =
-    pip freeze --exclude={env:PY_MODULE}
+    python -c 'print()'
+    pip freeze --exclude="{env:PY_MODULE}"
+
+    python -c 'print()'
+    pipdeptree


I am happy to see pipdeptree here :)

adiroiban · 2021-02-17T22:44:05Z

.github/workflows/cicd.yml

@@ -178,17 +178,27 @@ jobs:

    needs: [lint, mypy, docs, packaging]


This is a question of taste.

I think that is good to reduce wasted test runs... but quite often I want to see the full errors of a commit, so that I can fix them all at once and then push a new general fix.

But for example, I have workflows in which if the Ubuntu tests fail, Win and macOS tests are not executed (to save money and save the environment)

I would prefer to run the ubuntu tests, even if mypy is not 100% happy :)

In Travis, I had it set up so 1-2 unit test runs would go anyway, but that wasn't easily translated to GHA.

I may figure that out again later, but at the moment, this seems good enough.

I don't know if it's an issue with GHA, but I always found it annoying that I couldn't get a Travis result for a push because a million Twisted builds were queued up and many of them were wasting cycles on already-failed builds, and I think waste is waste so if you can't even mass listing steps, no need to try the unit test runs; it's not meaningfully slowing me down here.

Also: all of those steps run pretty quickly locally.

wsanchez · 2021-02-18T00:06:28Z

@adiroiban Thanks for the review

wsanchez added 2 commits February 9, 2021 16:15

pip doesn't like the git+git URL

c3c95c2

Add trunk

bf37ed3

wsanchez self-assigned this Feb 10, 2021

wsanchez requested a review from a team as a code owner February 10, 2021 00:16

Change CI badge from Travis to GHA

b990872

adiroiban reviewed Feb 10, 2021

View reviewed changes

wsanchez marked this pull request as draft February 10, 2021 03:17

wsanchez added 8 commits February 15, 2021 15:42

Don't test trunk because it's a hard failure.

6f02e94

Try to simplify based on treq's strategy

527df3b

Merge branch 'master' into ci-cleanup

c3592a2

Fix tox env bits

4e64044

Make dependencies env a bit more useful

2bd551c

Merge branch 'ci-cleanup' of github.com:twisted/klein into ci-cleanup

4f796e2

pypypy ➜ pypy

8918a96

+coverage

d9889a2

wsanchez added 2 commits February 16, 2021 13:29

Add fetch-depth: 2

d399241

Add os to matrix

111ac56

wsanchez commented Feb 16, 2021

View reviewed changes

wsanchez marked this pull request as ready for review February 16, 2021 21:53

wsanchez added 3 commits February 16, 2021 13:55

Merge branch 'master' into ci-cleanup

0cb0590

update dependencies

493cb59

Toss unsupported Twisted versions

a89c7ec

wsanchez marked this pull request as draft February 17, 2021 00:26

wsanchez added 2 commits February 16, 2021 16:37

See if we can get an optional trunk run in

6a0dd9b

syntax

3e22097

wsanchez commented Feb 17, 2021

View reviewed changes

wsanchez marked this pull request as ready for review February 17, 2021 06:11

adiroiban approved these changes Feb 17, 2021

View reviewed changes

wsanchez merged commit 4e6bb27 into master Feb 18, 2021

wsanchez deleted the ci-cleanup branch February 18, 2021 00:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI cleanup #450

CI cleanup #450

wsanchez commented Feb 10, 2021

adiroiban left a comment •

edited

Loading

adiroiban Feb 10, 2021

wsanchez commented Feb 10, 2021

codecov bot commented Feb 16, 2021 •

edited

Loading

wsanchez Feb 16, 2021

wsanchez Feb 16, 2021

adiroiban Feb 17, 2021

wsanchez Feb 16, 2021

wsanchez Feb 16, 2021

wsanchez Feb 16, 2021

wsanchez Feb 16, 2021

wsanchez Feb 16, 2021

adiroiban Feb 17, 2021

wsanchez Feb 17, 2021

wsanchez commented Feb 17, 2021

wsanchez commented Feb 17, 2021

wsanchez commented Feb 17, 2021

adiroiban left a comment •

edited

Loading

adiroiban Feb 17, 2021

adiroiban Feb 17, 2021

adiroiban Feb 17, 2021

adiroiban Feb 17, 2021

wsanchez Feb 18, 2021

wsanchez Feb 18, 2021

wsanchez commented Feb 18, 2021

	twisted: ["19.2", "current", "trunk"]
	twisted: ["19.2", "current", "trunk"]
	fail-fast: false

		.. image:: https://github.com/twisted/klein/workflows/CI%2fCD/badge.svg
		:target: https://github.com/twisted/klein/actions

		timeout-minutes: 30
		continue-on-error: ${{ matrix.optional }}

		@@ -178,17 +178,27 @@ jobs:

		needs: [lint, mypy, docs, packaging]

CI cleanup #450

CI cleanup #450

Conversation

wsanchez commented Feb 10, 2021

adiroiban left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsanchez commented Feb 10, 2021

codecov bot commented Feb 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsanchez commented Feb 17, 2021

wsanchez commented Feb 17, 2021

wsanchez commented Feb 17, 2021

adiroiban left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsanchez commented Feb 18, 2021

adiroiban left a comment •

edited

Loading

codecov bot commented Feb 16, 2021 •

edited

Loading

adiroiban left a comment •

edited

Loading