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.

Sign up for GitHub

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

Jump to bottom

ci(BA-542): Verify that the current model schema and the migration file match #2716

Merged
merged 11 commits into from
Feb 20, 2025
Merged

ci(BA-542): Verify that the current model schema and the migration file match #2716

merged 11 commits into from
Feb 20, 2025

Conversation

Yaminyam
Copy link
Member

@Yaminyam Yaminyam commented Aug 14, 2024 •
edited
Loading

Validate that the alembic revision file matches the current model schema by checking that no changes were made when it was created.
The require:db-migration changes the trigger condition of check-alembic-migration because it is a label that we attach if there is a change in the actual revision file.

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version

@Yaminyam Yaminyam added this to the 24.09 milestone Aug 14, 2024
@Yaminyam Yaminyam requested a review from achimnol August 14, 2024 11:25
@github-actions github-actions bot added the size:M 30~100 LoC label Aug 14, 2024
@Yaminyam Yaminyam added the skip:changelog Make the action workflow to skip towncrier check label Aug 14, 2024
achimnol
achimnol requested changes Aug 15, 2024
View reviewed changes
@Yaminyam Yaminyam requested a review from achimnol August 16, 2024 04:06
@Yaminyam Yaminyam changed the title ci: Verify that the current model schema and the migration file match ci(BA-542): Verify that the current model schema and the migration file match Jan 20, 2025
achimnol
achimnol requested changes Jan 20, 2025
View reviewed changes
HyeockJinKim
HyeockJinKim approved these changes Feb 7, 2025
View reviewed changes
Copy link
Collaborator

@HyeockJinKim HyeockJinKim left a comment

Choose a reason for hiding this comment

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

lgtm

@achimnol
Copy link
Member

achimnol commented Feb 7, 2025 •
edited
Loading

This is not what I meant....
I'd like to exclude the possibility of false-positive when there is a pass statement with a long body of downgrade() or upgrade() functions auto-generated or after edited by the PR authors to include custom logic.
My intention was to check if downgrade() and upgrade() functions are actually empty, not just they contain pass somewhere inside.

@Yaminyam

@achimnol achimnol added this pull request to the merge queue Feb 20, 2025
Merged via the queue into main with commit 14edde9 Feb 20, 2025
20 checks passed
@achimnol achimnol deleted the ci/check-alembic-revision branch February 20, 2025 15:06
lablup-octodog pushed a commit that referenced this pull request Feb 20, 2025
@Yaminyam @HyeockJinKim
ci(BA-542): Verify that the current model schema and the migration fi…
2e80d15 
…le match (#2716)

Co-authored-by: HyeockJinKim <kherootz@gmail.com>
Backported-from: main (25.3)
Backported-to: 24.09
Backport-of: 2716
@lablup-octodog lablup-octodog mentioned this pull request Feb 20, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Feb 20, 2025
@lablup-octodog @Yaminyam @HyeockJinKim
ci(BA-542): Verify that the current model schema and the migration fi…
86557a6 
…le match (#2716) (#3784)

Co-authored-by: Sion Kang <siontama@gmail.com>
Co-authored-by: HyeockJinKim <kherootz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achimnol achimnol achimnol approved these changes

@HyeockJinKim HyeockJinKim HyeockJinKim approved these changes

Assignees

@Yaminyam Yaminyam

Labels
size:M 30~100 LoC skip:changelog Make the action workflow to skip towncrier check
Projects
None yet
Milestone
24.09
Development

Successfully merging this pull request may close these issues.
3 participants
@Yaminyam @achimnol @HyeockJinKim