kpi-web-guild / django-girls-blog-OlenaEfymenko

django-girls-blog-OlenaEfymenko created by GitHub Classroom
MIT License
1 stars 0 forks source link

Add integration with GitHub Actions CI/CD #17

Closed OlenaYefymenko closed 1 year ago

OlenaYefymenko commented 1 year ago

Integration of CI/CD configuration into the project enables the running pre-commit tool. It provides fixing errors and following the best practices in the project. Setting up the badge into project README enables to display the current status of this workflow.

Resolves #14

webknjaz commented 1 year ago
  • to fetch the source code of the repository into workflow environment
  • to set up Python 3.11 and provide version compatibility for the workflow
  • to run pre-commit tool

There's no need to describe how the CI works. Virtually any CI setup works the same. Just mention that the CI is set up to run pre-commit on certain events and that's it.

OlenaYefymenko commented 1 year ago

Thank you, I edited as per your comments. But, I'm really not sure that I can add commit "Add README.md" in this PR. Also, I expect conflict with #13, because of the same document will be edited.

webknjaz commented 1 year ago

Don't add that commit. Only add README with the badge but not the rest of the content.

webknjaz commented 1 year ago

Add GitHub Actions CI/CD badge in README.md

Same as in another PR — use less specific file names.

Add GitHub Actions workflow files

One file, not many. But also just a workflow, it's obvious that you work with files in Git. You cannot commit changes that are not in files so this information is redundant. You could explain that it's running pre-commit tool in the commit long description. Also, point out why no-commit-to-branch is to be skipped in CI.

webknjaz commented 1 year ago

Oh, and it's usually nicer to keep natural order of the commits. There's no CI runs to link to until it's integrated so I'd expect the badge pointing there to be added in the second commit, not before the CI setup exists.

OlenaYefymenko commented 1 year ago

Updated the commits. But, I have some difficulties understanding the expediency of this:

env:
        SKIP: no-commit-to-branch

From asottile found an answer:

Looks like this hook false-positives when not on a branch (as is likely in CI) https://github.com/pre-commit/pre-commit-hooks/issues/265

Could you please clarify this issue?

webknjaz commented 1 year ago

Could you please clarify this issue?

The CI runs on PR events and push events. When you update a PR, a pull_request event happens and the CI job runs in the context of the incoming branch so the check would be green. OTOH, a push event happens when you push to branches which is fine for the PR branches too but merging PRs into the main branch also triggers a push event inside the GitHub's platform, which causes another CI job run but in the context of the main branch. This check would report failure in such case since it's a push to the main branch and we don't want that since it doesn't makes sense to validate that the commit is into the main branch on remote — it's already there. This specific check is a helper for local Git repositories preventing humans from accidentally committing to the main on their laptops. With branch protection on, they won't be able to push that to remote anyway. And if they try to git pull, it may create an ugly local merge commit (from origin/main to main) that they would need to clean up. This would also cause confusion for inexperienced contributors. The assumption is that one would run pre-commit install in their local repository and this small check would abort their commit attempts early, reminding them to switch branches. You can install it to run as a local pre-commit hook, once #15 is merged.

OlenaYefymenko commented 1 year ago

Could you add the missing motivation bit to this explanation? It's not a display. Just an indicator/badge/information. Try rephrasing this.

Maybe this variant will be better

webknjaz commented 1 year ago

@OlenaYefymenko well, it still doesn't explain the check exclusion.

OlenaYefymenko commented 1 year ago

@webknjaz ok, I considered you meant motivation for configuration integration. Maybe this variant for commit will be correct:

Integrate CI/CD configuration into project

This patch provides automation of the workflow — the running pre-commit tool. It enables prevention and fixing errors, following the best practices in the project. However, the no-commit-to-branch check should be excluded — merging PRs into the main branch would trigger incorrect failure.

Resolves ...

webknjaz commented 1 year ago

@OlenaYefymenko yep, that'll do. Maybe change "incorrect" to "false-positive", though.