Revert "reuse built frontend in ci, merge compose files (#6674)"

[eradman]

What type of PR is this?

• Bug Fix
• Revert refactoring

Description

Revert all commits >= 3f19534

These changes were far to invasive to test properly.

Reverted command used:

git reset --hard 24dec192eed8c95560ef20f6765d96c67a5bdbbf
git reset --soft HEAD@{1}

[AndrewChubatiuk]

@eradman what is a reason for this PR?

[eradman]

@AndrewChubatiuk the server does not start up correctly after these set of changes. As posted in the commit message:

Current:

IMAGE                                PORTS
redash-server                        0.0.0.0:5000->5000/tcp, :::5000->5000/tcp, 0.0.0.0:5678->5678/tcp, :::5678->5678/tcp
redis:7-alpine                       6379/tcp
pgautoupgrade/pgautoupgrade:latest   0.0.0.0:15432->5432/tcp, :::15432->5432/tcp
maildev/maildev                      0.0.0.0:1025->1025/tcp, :::1025->1025/tcp, 0.0.0.0:1080->1080/tcp, :::1080->1080/tcp

Expected:

IMAGE                                PORTS
redash-worker                        5000/tcp
redash-scheduler                     5000/tcp
redash-server                        0.0.0.0:5678->5678/tcp, :::5678->5678/tcp, 0.0.0.0:5001->5000/tcp, :::5001->5000/tcp
redis:7-alpine                       6379/tcp
maildev/maildev                      0.0.0.0:1025->1025/tcp, :::1025->1025/tcp, 0.0.0.0:1080->1080/tcp, :::1080->1080/tcp
pgautoupgrade/pgautoupgrade:latest   0.0.0.0:15432->5432/tcp, :::15432->5432/tcp

Commit 3f19534 is a large change, and needs to broken into separate commits so that we can more easily test it and evaluate changes to behavior.

[AndrewChubatiuk]

for a local setup you need to use docker compose local profile

docker compsoe --profile local up -d

I've added a PR, where added some fixes. #6877
It'll take less time to fix, than to revert

[eradman]

The dev instructions need to be updated if make up is not used any more.

Also, the server appears to be reachable on port 5000, not port 5001 as it was before.

[AndrewChubatiuk]

The dev instructions need to be updated if make up is not used any more.

Also, the server appears to be reachable on port 5000, not port 5001 as it was before.

updated makefile in #6877 and changed port number in Readme to 5000

[justinclift]

If we get the Makefile issues sorted out + change the port number back to 5001, would that be ok with you @eradman?

A lot of the changes in the commit this is reverting seemed pretty rational. The port number change is an easy one to change back though. 😄

[eradman]

@justinclift if make up works again then I'd say we can proceed.

Moving the port back to 5001 is a good idea, but not necessary.

Also, node:18.1-bookworm does not resolve for me

$ make
docker compose build
[+] Building 0.7s (4/4) FINISHED
 => [server internal] load build definition from Dockerfile                               0.0s
 => => transferring dockerfile: 3.54kB                                                    0.0s
 => [server internal] load .dockerignore                                                  0.1s
 => => transferring context: 203B                                                         0.0s
 => CANCELED [server internal] load metadata for docker.io/library/python:3.8-slim-bookw  0.6s
 => ERROR [server internal] load metadata for docker.io/library/node:18.1-bookworm        0.6s
------

[justinclift]

No worries. 😄

Looking at the tags lists on the Node page on Docker Hub, we should probably go with 18-bookworm. There are few variations of that (eg 18.20.2-bookworm) so I'm guessing the 18.1-bookworm thing was a typo.

[justinclift]

I've just directly pushed a commit to master to fix the Dockerfile image for NodeJS: 9601660

[justinclift]

@eradman The follow up PR (#6877) should resolve the technical problems. It also makes the Redash webUI port configurable too.

We still need to update our wiki docs, which will be over the next few hours.

[eradman]

The port should be changed back to 5001. The build succeeds now, but Cypress will not run

Cypress could not verify that this server is running:

  > http://localhost:5001

[justinclift]

@eradman Good catch. The port number Cypress is trying to connect to comes from here:

redash/cypress.config.js

Line 5 in c12d450

baseUrl: 'http://localhost:5001',

That being said, I'm in favour of updating the default port to be 5001 too, especially after changing the setup.sh script to use that a few days ago. 😄

I'll throw something together shortly to get that done. Getting it done in #6883.

[justinclift]

@eradman The default port for the webUI has been changed back to 5001 now (062a70c). 😄

[eradman]

Build and tests work now from master. Closing

[eradman]

Reopening since I continue to spend time (days) debugging problems with this.

The latest issue is that the server does not auto-reload when changes are made, probably because of the changes to the docker volume mounts.

[AndrewChubatiuk]

where do you expect auto reload?
why don't you create an issue instead?
lot's of useful progress and fixes were done in this PR and in the work after it to revert it just because of local env issues, which should be easy to fix

[AndrewChubatiuk]

need to add REDASH_PRODUCTION=false to .env file. it'll use other scripts for services, which support hot reload

[eradman]

why don't you create an issue instead?

I will some issues as well

[eradman]

Updated this PR to include all changes from April 8, 2024 - April 29.

[AndrewChubatiuk]

@eradman please try these changes
#6933
it should help with hot reload on a server

[eradman]

Rebasing to resolve conflicts. It is still my view that we should revert to the last stable condition.

[AndrewChubatiuk]

@eradman what issues do you have? you do not maintain any communication, so it's impossible to understand your point. if you local environment is not working it doesn't mean it's not stable

[justinclift]

It is still my view that we should revert to the last stable condition.

Ouch. That'll be a pretty major reversion, but not impossible if there's serious stuff happening that doesn't look resolvable.

[eradman]

It is still my view that we should revert to the last stable condition.

Ouch. That'll be a pretty major reversion, but not impossible if there's serious stuff happening that doesn't look resolvable.

For context: I have used the major part of three weeks hitting a series of new problems, some of which have been reported and fixed

#6883
#6927
#6937

Some of which still need work

#6933
#6940

I don't have more time (in the short term) to test fixes to functionality that existed prior to April 10, so reverting is the only way I can think to make progress with available resources.

[AndrewChubatiuk]

@eradman for both issues you've mentioned, there're PR's with fixes and they are ready
#6940
#6933

[justinclift]

@eradman Understood. I'll take a look at rolling things back tomorrow (too sleepy right now), then figure out what other PRs we'll probably need to apply on top of it afterwards. There's been a few PRs that seem constructive and useful (dependabot, some of mine, and some others that seem useful).

[justinclift]

As a general note to everyone, this "reverting back to a known working state" approach is likely to go ahead.

We (@getredash/maintainers) are just figuring out how to do it well.

It might take us a few days to really work out the right process. 😄

---

Further reviews of PRs before that's completed is kind of useless, so don't anyone expect reviews to be happening over the next few days.

I'm not real sure how well that's going to fit any currently happening reviews (sorry @masayuki038). Might be best to hold off on those until this large PR reverting is finished.

[eradman]

Will try to refactor this as a series of commits to give the CI a chance to pass.