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

feat: target platform e2e #3340

Closed
wants to merge 27 commits into from
Closed

feat: target platform e2e #3340

wants to merge 27 commits into from

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Jan 2, 2023

Contributes to prisma/prisma#14017, #3332, prisma/prisma#16962, prisma/docs#4303

This PR adds Dockerised OS Support tests for in the ./docker folder. In particular, it tries to run Prisma Client (both binary and library) in the following systems:

  • alpine-3.16-amd64-openssl-1.1.x
  • alpine-3.17-amd64-openssl-3.0.x
  • alpine-latest-amd64-openssl-3.0.x
  • debian-bullseye-amd64-openssl-1.1.x
  • debian-buster-amd64-openssl-1.1.x
  • debian-latest-amd64-openssl-1.1.x
  • opensuse-tumbleweed-amd64-openssl-1.1.x
  • ubuntu-20.04-amd64-openssl-1.1.x
  • ubuntu-22.04-amd64-openssl-3.0.x
  • ubuntu-latest-amd64-openssl-3.0.x
  • ubuntu-latest-arm64-openssl-3.0.x

Additionally, for each Dockerised system, we assert the binaryTarget selected by Prisma (i.e., the Current platform value in prisma -v). If a major Docker image suddenly introduces a new OpenSSL version we don't support, or switches to another architecture by default, we will receive a Slack notification.

Multiarchitecture support (amd64, arm64) works via QEMU (docker/setup-qemu-action@v2) and Docker Buildx (docker/setup-buildx-action@v2).

Review Helpers

TODOs

From these tests, I have opened the following issues:

We should:

  • Solve the issues above
  • Add debian-latest-arm64-openssl-1.1.x to the Docker test pipeline once https://github.com/prisma/prisma-private/issues/205 is solved.
  • Run docker workflow only once per day
  • Add failure expectation for the following systems:
    • _fail-debian-buster-amd64-openssl-1.1.x

Comment on lines +206 to +214
- name: Set up QEMU
uses: docker/setup-qemu-action@v2
with:
platforms: "arm64"

- name: Set up Docker Buildx
id: buildx
uses: docker/setup-buildx-action@v2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add multiarchitecture support (amd64, arm64)

@jkomyno jkomyno requested a review from Jolg42 January 3, 2023 12:53
@jkomyno jkomyno added this to the 4.9.0 milestone Jan 3, 2023
@jkomyno jkomyno marked this pull request as ready for review January 3, 2023 12:54
Comment on lines +191 to +192
# Add back once we solve https://github.com/prisma/prisma-private/issues/205
# - debian-latest-arm64-openssl-1.1.x
Copy link
Member

Choose a reason for hiding this comment

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

We could also have it before we fix it? (it would fail, but we'll instantly know once it's fixed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but I'd be wary of merging something that results in a red CI status

docker/README.md Outdated Show resolved Hide resolved
@jkomyno
Copy link
Contributor Author

jkomyno commented Jan 3, 2023

Note: change base to dev

@jkomyno jkomyno changed the base branch from latest to dev January 3, 2023 16:00
@jkomyno jkomyno changed the base branch from dev to latest January 3, 2023 16:02
RUN ./uname.sh

# Only OpenSSL 1.1 is expected to be on this system
RUN ldconfig -p | grep ssl
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RUN ldconfig -p | grep ssl
RUN ldconfig -p | grep ssl | sed "s/.*=>\s*//"

Missing or intentional?

@jkomyno jkomyno mentioned this pull request Jan 3, 2023
4 tasks

FROM ubuntu:${UBUNTU_VERSION}

ARG NODE_VERSION="18.12.1"
Copy link
Member

Choose a reason for hiding this comment

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

We discussed that this could use the most recent LTS version and that we could have a test for the latest version too

Copy link
Member

@Jolg42 Jolg42 Jan 3, 2023

Choose a reason for hiding this comment

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

Easy way would be to replace with https://github.com/nodesource/distributions

Node.js LTS (v18.x):
Using Ubuntu

curl -fsSL https://deb.nodesource.com/setup_lts.x | sudo -E bash - &&\
sudo apt-get install -y nodejs

Using Debian, as root

curl -fsSL https://deb.nodesource.com/setup_lts.x | bash - &&\
apt-get install -y nodejs

Node.js Current (v19.x):
Using Ubuntu

curl -fsSL https://deb.nodesource.com/setup_current.x | sudo -E bash - &&\
sudo apt-get install -y nodejs

Using Debian, as root

curl -fsSL https://deb.nodesource.com/setup_current.x | bash - &&\
apt-get install -y nodejs

Copy link
Member

@Jolg42 Jolg42 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!
I checked the logs and everything seems to work like expected there 💯

We discussed we don't want to merge this PR because it's based on latest branch and we want to merge to dev.

@jkomyno
Copy link
Contributor Author

jkomyno commented Jan 3, 2023

Closing this in favor of #3341.

@jkomyno jkomyno closed this Jan 3, 2023
@janpio janpio deleted the feat/target-platform-e2e branch April 6, 2023 19:55
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.

None yet

2 participants