reuse built frontend in ci, merge compose files

[AndrewChubatiuk]

What type of PR is this?

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

Description

• renamed docker-compose.yml to compose.yml as it's supported by docker compose v2 by default
• merged all compose files to simplify local backend and e2e tests execution. added profiles:
  • local - spins up all services in a setup, that previously was in docker-compose.yml
  • e2e - spins up all services in a setup, that previously was in .ci/docker-compose.cypress.yml
  • default(no profile set) - spins up all services in a setup, that previously was in .ci/docker-compose.yml
• added ability to build frontend inside docker or reuse prebuilt frontend locally for CI purposes to speed up build
• added arm64 build

How is this tested?

• Unit tests (pytest, jest)
• E2E Tests (Cypress)
• Manually - executed e2e and unit tests locally against all three compose profiles
• N/A

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

[AndrewChubatiuk]

@justinclift could you please review this MR?

[justinclift]

@AndrewChubatiuk It's unlikely I'll have much free time to review Redash stuff today. 😦

Maybe one of the others can help instead? 😄

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.37%. Comparing base (af0773c) to head (15b94b9).
Report is 2 commits behind head on master.

❗ Current head 15b94b9 differs from pull request most recent head 1619b0b. Consider uploading reports for the commit 1619b0b to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6674      +/-   ##
==========================================
- Coverage   63.82%   63.37%   -0.45%     
==========================================
  Files         161      162       +1     
  Lines       13060    13170     +110     
  Branches     1803     1819      +16     
==========================================
+ Hits         8335     8347      +12     
- Misses       4425     4532     +107     
+ Partials      300      291       -9     

see 21 files with indirect coverage changes

[justinclift]

Thinking this bit over:

renamed docker-compose.yml to compose.yml as it's supported by docker compose v2 by default

I'm unsure if we want that rename. My thinking is that docker-compose.yml is widely known and recognised, while compose.yml isn't.

[justinclift]

Same question as above. 😉

[AndrewChubatiuk]

these values were just b64 encoded. need to move them to secrets

[AndrewChubatiuk]

I can remove these values if you can move them to secrets

[justinclift]

This sounded familiar, so after some looking I've found previous discussion about those values:

f3892e0#commitcomment-120444257

Looks like they were put there on purpose when we were previously using CircleCI.

... this was intentional for CircleCI, because secrets don't work with PRs from Forked repositories.

We've moved from CircleCI to GitHub Actions, but the same thing might apply. We'd probably want to double check though, just to be sure.

[AndrewChubatiuk]

github actions with pull_request event type have the same behavior as in CircleCI, but we can try pull_request_target, which requires more attention to PR before running it, cause it provides a token with a write access to base repo, but also provides an access to base repo secrets

[AndrewChubatiuk]

@justinclift could you please review it again?

[AndrewChubatiuk]

@guidopetri @justinclift @eradman could you please review this MR?
With pull_request_target event type (which redash was recently switched to) Github Actions uses a pipeline from a target(master) branch, so this MR will never be green

[eradman]

When I run the build there are a number of new warnings:

$ make
docker compose build
WARN[0000] The "COMMIT_INFO_SHA" variable is not set. Defaulting to a blank string.
WARN[0000] The "COMMIT_INFO_REMOTE" variable is not set. Defaulting to a blank string.
WARN[0000] The "CYPRESS_PROJECT_ID" variable is not set. Defaulting to a blank string.
WARN[0000] The "PERCY_TOKEN" variable is not set. Defaulting to a blank string.
WARN[0000] The "PERCY_BRANCH" variable is not set. Defaulting to a blank string.
WARN[0000] The "COMMIT_INFO_AUTHOR" variable is not set. Defaulting to a blank string.
WARN[0000] The "COMMIT_INFO_BRANCH" variable is not set. Defaulting to a blank string.
WARN[0000] The "COMMIT_INFO_MESSAGE" variable is not set. Defaulting to a blank string.
WARN[0000] The "CYPRESS_RECORD_KEY" variable is not set. Defaulting to a blank string.
WARN[0000] The "PERCY_COMMIT" variable is not set. Defaulting to a blank string.
WARN[0000] The "PERCY_PULL_REQUEST" variable is not set. Defaulting to a blank string.
WARN[0000] The "REDASH_PRODUCTION" variable is not set. Defaulting to a blank string.
WARN[0000] The "REDASH_PRODUCTION" variable is not set. Defaulting to a blank string.
WARN[0000] The "REDASH_PRODUCTION" variable is not set. Defaulting to a blank string.

The build works, but these messages make it look like I am missing a step. I'd say we should only emit a warning if the developer has an incomplete configuration.

[AndrewChubatiuk]

@eradman made these variable overridable and empty by default to remove warnings

[justinclift]

Interesting. Hadn't come across this docker setting before. Looks useful to know about for some situations. 😄

https://docs.docker.com/reference/cli/docker/container/run/#ipc

[justinclift]

This looks 95% good to me. The only two important things that I think need changing before it can be merged are:

• Change the migrate name back to create_db in the docker entrypoint

  • And whatever calls that name of course

• Remove the change of REDASH_REDIS_URL environment variable

  • It's already documented using this name

• Remove the change of REDASH_DATABASE_URL environment variable

  • It's already documented using this name

For updating this, please just add new commits so I don't have to re-review the entire thing. 😄

[justinclift]

99% there now. Just needs the Makefile target changed in order to merge this. 😄

[justinclift]

Looks good to me. 😄

[justinclift]

Ahhh, yeah. We might as well include the lint change too. All good. 😄