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

Fix docker migration test CI workflow (PP-1171) #1800

Merged
merged 5 commits into from
Apr 19, 2024
Merged

Conversation

jonathangreen
Copy link
Member

@jonathangreen jonathangreen commented Apr 19, 2024

Description

Update docker/ci/test_migrations.sh:

  • Output more debug information, so this is easier to troubleshoot next time
  • Format that output, so its easy to see whats going on in gh actions
  • Don't try to build a container for the old version of the CM that we need to run
    • Instead of building a container, we just use an old container image that was already built
    • To make this easier a new docker-compose file test_migrations.yml was added for this purpose

Motivation and Context

Previously we tried to build a container based on the old version of the codebase for these tests. That is an error prone endeavor, and failed in this case because of the change in dependencies in the container build introduced in #1799.

In order to reduce the chances of this failing in the future, we just use an old previously build version of the container. This also simplifies the script, since it no longer has to move back and forth between old and new git commits in order to run the tests.

How Has This Been Tested?

  • Running script locally
  • Running in CI

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@jonathangreen jonathangreen marked this pull request as ready for review April 19, 2024 16:20
@jonathangreen jonathangreen requested a review from a team April 19, 2024 16:34
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.30%. Comparing base (2473400) to head (db1347a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1800      +/-   ##
==========================================
- Coverage   90.31%   90.30%   -0.01%     
==========================================
  Files         259      259              
  Lines       28663    28663              
  Branches     6515     6515              
==========================================
- Hits        25887    25885       -2     
- Misses       1839     1840       +1     
- Partials      937      938       +1     
Flag Coverage Δ
Api 75.57% <ø> (ø)
Core 59.79% <ø> (-0.01%) ⬇️
migration 24.08% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@dbernstein dbernstein left a comment

Choose a reason for hiding this comment

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

@jonathangreen : I just never stop admiring your skillz, dog. 🥇 You set a high bar.

Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

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

Looks good! ⬆️ ⬇️ ⬆️ ▶️ 🥳

Copy link
Contributor

@dbernstein dbernstein left a comment

Choose a reason for hiding this comment

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

Not sure if the issue I pointed out is a problem. Feel free to merge if it is not.

check_db "A new migration is required or a up migration is broken."
gh_group "Testing upgrade migrations"
run_migrations "webapp" "upgrade head"
check_db "webapp" "A new migration is required or a up migration is broken."
Copy link
Contributor

Choose a reason for hiding this comment

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

a -> an

run_migrations "upgrade head"
check_db "A new migration is required or a up migration is broken."
gh_group "Testing upgrade migrations"
run_migrations "webapp" "upgrade head"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be" webapp-old" here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can't be webapp-old because the old container doesn't have the migrations in it in order to move to the latest DB version.

Copy link
Contributor

@dbernstein dbernstein Apr 19, 2024

Choose a reason for hiding this comment

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

okay - right of course: I get it now - one database container is shared by the two web app instances.

@jonathangreen jonathangreen merged commit 0bd161d into main Apr 19, 2024
24 checks passed
@jonathangreen jonathangreen deleted the feature/fix-ci branch April 19, 2024 17:53
ttuovinen added a commit to NatLibFi/ekirjasto-circulation that referenced this pull request Apr 22, 2024
ttuovinen added a commit to NatLibFi/ekirjasto-circulation that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants