Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[DI-2709] Adds CI worflow for the generated project #89
[DI-2709] Adds CI worflow for the generated project #89
Changes from 1 commit
55afefa
ecf72d8
4a1c293
e8764dd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.