Nitrokey / pynitrokey

Python client for Nitrokey devices
Apache License 2.0
98 stars 27 forks source link

CI pipeline for checks on PR creation and branch pushes #357

Closed mmerklinger closed 1 year ago

mmerklinger commented 1 year ago

This PR adds a CI pipeline to run checks on pushes to non-master branches and pull request.

Changes

Checklist

Test Environment and Execution

Relevant Output Example

Fixes #

daringer commented 1 year ago

this is the path towards github built .msi and .exe releases

mmerklinger commented 1 year ago

this is the path towards github built .msi and .exe releases

@daringer This PR will only provide the CI pipeline. The CD pipeline will follow in another PR, after a bit more testing. Hence, this PR is complete and ready for review.

daringer commented 1 year ago

please have a short look here @szszszsz & @robin-nitrokey ... 0.4.35 is now released, I would suggest we target 0.5 very soon(tm) including this github CI pipeline and the matching release pipeline #361

mmerklinger commented 1 year ago

Looks good to me. Why are the checks split into separate jobs though?

They are separate for three reasons:

  1. With the current implementation you know the name of the check from the job name if one fails.
  2. The error log can be long and hard to read when you have multiple failing checks in one job.
  3. Separate jobs can run in parallel, which makes the feedback faster.
robin-nitrokey commented 1 year ago

As far as I see, the setup of the environment is the same for all tasks and takes much longer than actually running the tests, so I would default to keeping them in the same job. But I don’t have a strong opinion on it.

mmerklinger commented 1 year ago

As far as I see, the setup of the environment is the same for all tasks and takes much longer than actually running the tests, so I would default to keeping them in the same job. But I don’t have a strong opinion on it.

I would vote for keeping separate jobs mostly because of the number 1 and 2 of my reasoning mentioned above. Your argument about the time is valid. I was planning to look into using a cache to speed this up. I wanted this to test before though.

daringer commented 1 year ago

I was planning to look into using a cache to speed this up. I wanted this to test before though.

This should be the way to go to fulfill all mentioned requirements.

Let's merge this and #361 so we could also allow our customers to use some of the not officially released builds to update all Nitrokey 3 variants to v1.3.