add commit msg tag to reset dashboard db in drone#70434
Conversation
47b7af0 to
cb1f6fd
Compare
ecb0746 to
4b509f2
Compare
This reverts commit c161d72.
| Dir.chdir('dashboard') do | ||
| if CI::Utils.tagged?(RESET_DB_TAG) | ||
| ChatClient.log "Commit message: '#{CI::Utils.git_commit_message}' contains [#{RESET_DB_TAG}], resetting dashboard database." | ||
| RakeUtils.rake_stream_output 'db:reset db:setup_or_migrate' |
There was a problem hiding this comment.
Just checking - is this safe to do in Drone? I don't know how our caching works but I wouldn't like my PR that removes units from seeding to see what fails to cause everyone's PRs to fail 😅
There was a problem hiding this comment.
in theory it should be safe because the next step is to run seed:ui_test, and DB setup + seeding should get us back into the same state (modulo any updates made in the PR being tested) vs what existed in cached DB generated by the cache-staging-build pipeline (defined in .drone.yml). I'm not aware of any other database modifications that could have been made during the cache-staging-build step that could be lost here.
I attempted to validate this assumption via the results of the individual drone runs outlined in the PR description. if something in drone ui tests (the only place seed_ui_test is run) was broken by this, then I'd have expected this drone run to fail:
- 13ce738 ui tests still pass when new [reset db] tag is specified
as for making everyone's PRs fail, I think we can be confident that won't happen in this case because seed_ui_test does not get run in cache-staging-build. that pipeline calls a different method which does not hit this new codepath:
| # Reset the dashboard database before seeding for UI tests. By default, the | ||
| # database will have been prepopulated based on recent data from the staging | ||
| # branch (see cache-staging-build pipeline in .drone.yml). | ||
| RESET_DB_TAG = 'reset db'.freeze |
There was a problem hiding this comment.
Is it worth adding a commit hook like this one when someone updates dashboard/lib/tasks/seed.rake to suggest using this tag?
There was a problem hiding this comment.
I agree with the spirit of this and I'd be happy to better advertise or enforce this flag if it can be done in a high-signal manner, but I think this one could be a bit noisy because
- most people are going to be adding to the list of what's seeded, or sometimes touching parts of the file that have nothing to do with what gets seeded in UI tests
- the commit hook you linked to often gets triggered by other people's code changes when I'm doing a merge, and so the message isn't applicable to me
although I'm planning to use this tag pretty heavily in the next few weeks/months, I feel more comfortable with being a bit quieter about it because it doesn't seem relevant to most normal feature development, and folks who are doing cleanup projects can still find it in ci.rake if they need to.
There was a problem hiding this comment.
in the absence of a commit hook, I'll beef up the comment here to make it extra clear when this tag should probably be used.
bethanyaconnor
left a comment
There was a problem hiding this comment.
A question and a suggestion but the change looks good to me! Thanks for doing this!
See #66876 (comment) for a description of the original issue -- Bethany tried removing some units from UI_TEST_SCRIPTS which she knew were being depended on by UI tests in order to get a list of failures, but the drone run unexpectedly passed. The underlying issue is that the
cache-staging-buildpipeline in.drone.ymlhas already prepopulated the database with those courses / units and cached them. So, when entries were removed fromUI_TEST_SCRIPTSin a pending PR, this has no effect on the drone run, because those units already exist in the drone run's database.The fix proposed by this PR is to add a new
[reset db]commit message tag (similar to existing[test all]or[skip ui]tags). This causes the database to be dropped and recreated during the ui test setup for that particular drone run.Depends on:
.seededfile (generated by the cached staging build) tricks us into skipping seeding for all units, and then course seeding fails.Testing story
validated via drone runs on intermediate commits, which show the following:
[reset db]tag is specified[reset db]tag, removing a course / unit from seeding does not break UI tests. this is the original bug[reset db], removing course / unit does break UI tests. this is the desired behaviorFuture work
The new behavior could be triggered automatically when there are changes to
dashboard/lib/tasks/seed.rake, however this could be a bit heavy-handed given the 27-minute penalty for re-seeding the database.