Skip to content
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

Comment for isolated migrations #15760

Closed

Conversation

saraycp
Copy link
Contributor

@saraycp saraycp commented Mar 7, 2024

The Isolated Migrations check doesn't cover all the project's necessities:

  • it fails when we commit the models' annotations together with the migrations,
  • it fails when we commit validations and their corresponding database consistency fix (which should go together),
  • ...

There is no need to have a check that fails or passes. It is enough with a comment warning about the presence of migrations together with other changes.

With these two new GitHub actions, the PR can get any of the comments in the following image when:

  • there are migrations together with other changes (except schema changes)
  • there are migrations without their corresponding schema

Screenshot 2024-03-11 at 23-16-05 WIP warn migration by saraycp · Pull Request #29 · saraycp_hello_world

Pull requests that come from a fork don't have write permissions, so we can't write comments to a PR from one single GitHub action. That's why this is split into two actions to make it work and be safe, according to this article.

Moreover, these actions should be in production to work. Otherwise, the second action/workflow will never run.

@github-actions github-actions bot added the Test Suite / CI 💉 Things related to our tests/CI label Mar 7, 2024
@saraycp saraycp force-pushed the comment_for_isolated_migrations branch 21 times, most recently from 1ce5771 to a7d5d5c Compare March 12, 2024 18:31
Copy link
Contributor

@krauselukas krauselukas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saraycp I think you have to do something similar like this #14159 (see the PR description for explanation) in order to resolve the permission issues. Splitting this workflow into two separate one's and use the :pull_request_target scope for the commenting part.

@saraycp saraycp force-pushed the comment_for_isolated_migrations branch 2 times, most recently from fde5576 to a4df352 Compare March 18, 2024 11:04
@saraycp saraycp marked this pull request as ready for review March 18, 2024 11:13
They write comments when:
  - there is a migration together with other changes (except schema
    changes),
  - there is a migration but not its corresponding db schema,
  - there is a data migration but not its corresponding data schema.

It is splitted into two actions, the first one stores the texts in
artifacts (text files) and the second one create the comments using
those texts. This is required to allow pull requests that come from
forks to write.
@saraycp saraycp force-pushed the comment_for_isolated_migrations branch from a4df352 to f8b1418 Compare March 18, 2024 12:16
@saraycp
Copy link
Contributor Author

saraycp commented Mar 18, 2024

Closing in favor of: #15824

@saraycp saraycp closed this Mar 18, 2024
@saraycp saraycp deleted the comment_for_isolated_migrations branch March 19, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Test Suite / CI 💉 Things related to our tests/CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants