Automatic database upgrades upon services start-up

[zachliu]

What type of PR is this?

• Refactor
• Feature
• Bug Fix
• New Query Runner (Data Source)
• New Alert Destination
• Other

Description

Automatically upgrade the database upon services start-up. So that we don't need to do it manually using cli tools on the server.

I started the discussion #7005 on automated db migration mechanisms.

Further discussing: #7161 (comment)

How is this tested?

• Unit tests (pytest, jest)
• E2E Tests (Cypress)
• Manually
• N/A

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

[justinclift]

Ahhh. This looks like it's causing a CI failure by attempting a database upgrade even before the database has been initialised.

@zachliu Are you ok to investigate and get it working? 😄

[zachliu]

@justinclift we probably need a "wait" in ci.yml between these two commands

redash/.github/workflows/ci.yml

Lines 162 to 163 in c42b151

	yarn cypress start -- --skip-db-seed
	docker compose run cypress yarn cypress db-seed

do you prefer a blunt sleep 35 or a smarter .ci/wait-for-it.sh localhost:5001 --strict --timeout=35 -- echo "Server is ready."?

[justinclift]

Hmmm, I wonder why we're skipping the database seed on one line, then running it manually on the next line? At some point I probably knew the answer to that. 😉

That being said, of your two options we should probably take the smarter 😄 approach and wait for the previous step to actually be complete.

[zachliu]

actually, redash has been using a simple sleep in the ci.yml file:

redash/.github/workflows/ci.yml

Lines 50 to 52 in c42b151

	docker compose up -d
	sleep 10
	- name: Create Test Database

i figure i'll just keep it simple and consistent with the existing code 😁

dumb question: i'll need another PR to update the CI right? otherwise it won't take effect

[gaecoli]

Is this change too dangerous?

[justinclift]

Is this change too dangerous?

In theory (!) it shouldn't be, at least for not-big-version-jumps (of Redash version).

People trying to run the development code directly against a Redash database from prior-to-version-10 might have issues though (am guessing).

But that would be a very unexpected situation for someone to try unless intentionally getting creative for some reason. 😉

[justinclift]

i figure i'll just keep it simple and consistent with the existing code 😁

No worries.

dumb question: i'll need another PR to update the CI right? otherwise it won't take effect

Nah. In theory the CI runs from the state of the repo as it is presented by the PR itself.

ie it'll make a git clone -b [PR branch] onto the chosen CI server and then run the CI from the files there.

At least, that's my present understanding of things.

Does that help? 😄

[gaecoli]

Is this change too dangerous?

In theory (!) it shouldn't be, at least for not-big-version-jumps (of Redash version).

People trying to run the development code directly against a Redash database from prior-to-version-10 might have issues though (am guessing).

But that would be a very unexpected situation for someone to try unless intentionally getting creative for some reason. 😉

OK!

[zachliu]

i figure i'll just keep it simple and consistent with the existing code 😁

No worries.

dumb question: i'll need another PR to update the CI right? otherwise it won't take effect

Nah. In theory the CI runs from the state of the repo as it is presented by the PR itself.

ie it'll make a git clone -b [PR branch] onto the chosen CI server and then run the CI from the files there.

At least, that's my present understanding of things.

Does that help? 😄

interesting, then it's very odd that the sleep 30 is ineffective and didn't show up in the log at all 🤔

yarn cypress start -- --skip-db-seed
sleep 30
docker compose run cypress yarn cypress db-seed

[justinclift]

Well, that's weird then. 😕

Maybe do some test PR's in a different fork or something along those lines?

[zachliu]

did some tests in my own fork and found the problem(s):

1. when updating the .github/workflows/ci.yml, the changes don't take effect until they are merged to master (they will be effective for future PRs)
2. the issue is not only because we need a sleep between these two commands

   yarn cypress start -- --skip-db-seed
   docker compose run cypress yarn cypress db-seed
   

   but also we are upgrading the db before creating it
   

   redash/client/cypress/cypress.js

   Lines 49 to 53 in c42b151

   	function startServer() {
   	console.log("Starting the server...");
   	execSync("docker compose -p cypress up -d", { stdio: "inherit" });
   	execSync("docker compose -p cypress run server create_db", { stdio: "inherit" });
   	}

opened two new PRs:

1. wait for the db init & upgrade #7167
2. Automatic database upgrades #7166

[justinclift]

the changes don't take effect until they are merged to master

Cool. 😄

Heh Heh Heh, I really thought it was whatever the state of the PR branch was that controlled it. But something every day I guess. 😄

we are upgrading the db before creating it

Makes sense. Not a race condition then, just an order-of-operation thing.

Good stuff @zachliu, I'll take a look at those when I'm at the co-working space later on today. 😁

[justinclift]

I'll take a look at those when I'm at the co-working space later on today.

It's going to have to be tomorrow instead. Today I went through the investigation of broken embeds instead (finding a good enough workaround), and now I'm mentally brain drained.

I've finished the embeds investigation now though, so this will be the first thing to look at tomorrow (unless something urgent pops up overnight). 😄