[DI-2709] Adds CI worflow for the generated project #89
Conversation
- Adds `.guthub/workflows/ci.yml` with couple jobs to: -- run pre-commit hooks on CI, inc. ruff linting & formatting; -- run `api` tests; - Update couple versions for pre-commit hooks
| # TODO: Run this hook from docker container. | ||
| # - repo: local | ||
| # hooks: | ||
| # - id: migrations-check | ||
| # language: system | ||
| # name: Check for uncreated migrations. | ||
| # entry: sh -c "make api-check-migrations" | ||
| # files: models/*.*\.py$ | ||
| # stages: [commit] |
There was a problem hiding this comment.
We will run this on CI, so I think it's OK to remove it from the pre-commit
There was a problem hiding this comment.
I don't think that pre-commit hook & CI are for the same purpose. In my point of view pre-commit hook is a helper for developer to fix and notify some stupid issues like formatting, unused imports & not created migrations.
Pre-commit hook is not required, while CI is. So, pre-commit hook helps to avoid unnecessary CI runs in case of some issues. It's better to get a notification about these issues when you make a commit instead of waiting for a CI run.
I would like to keep this hook.
There was a problem hiding this comment.
@alexryabtsev
I don't know why it was "commented" before, I can uncomment it and see if it works fine with Docker, though. If not, I can keep it commented and we can look at this later in another task/PR.
There was a problem hiding this comment.
I don't know either why it's commented out.
@SergeyKubrak do you remember the reason why it was commented here?
@bulya Please try to uncomment and test it, if you have a capacity. If it won't work, let's try to fix it in another task. Thank you.
There was a problem hiding this comment.
@bulya It's not a strong requirement. You can remove checking migrations from pre-commit hooks, if it produces additional complexity in CI workflow. It's more important to have this check in CI rather than in pre-commit hooks.
There was a problem hiding this comment.
I am leaving this for visibility.
We have nevertheless decided to remove this from the pre-commit hooks, as we run all pre-commit hooks in the linter CI job, and we don't want either to run it there via the docker-compose or explicitly exclude it from the hooks list.
Co-authored-by: Denys Chura <[email protected]>
Co-authored-by: Denys Chura <[email protected]>
Change list
.guthub/workflows/ci.ymlwith couple jobs to:apitests;safetypre-commit hook to work withpoetryThe workflow tested on the newly generated project, and looks like this on GH: