Skip to content

feat(mbe, ebe): dockerize mbe/ebe#25

Merged
mohammadalfaiyazbitgo merged 1 commit intomasterfrom
WP-4681/dockerize-onprem
Jun 16, 2025
Merged

feat(mbe, ebe): dockerize mbe/ebe#25
mohammadalfaiyazbitgo merged 1 commit intomasterfrom
WP-4681/dockerize-onprem

Conversation

@mohammadalfaiyazbitgo
Copy link
Copy Markdown
Contributor

Ticket: WP-4681

@mohammadalfaiyazbitgo mohammadalfaiyazbitgo marked this pull request as draft June 12, 2025 19:28
@mohammadalfaiyazbitgo mohammadalfaiyazbitgo changed the title wip(mbe): dockerize mbe feat(mbe, ebe): dockerize mbe/ebe Jun 14, 2025
@mohammadalfaiyazbitgo mohammadalfaiyazbitgo marked this pull request as ready for review June 14, 2025 20:45

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor

@pranavjain97 pranavjain97 left a comment

Choose a reason for hiding this comment

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

We should have different dockerfiles for both the apps to keep the dependencies limited to their app. Enclaved express should have a very light dockerfile.

Also, please lets add a docker-compose to ease up local dev, but you can do it in a follow-up.

Comment thread Dockerfile Outdated
org.opencontainers.image.revision=${VCS_REF}

# Set runtime environment
ENV NODE_ENV=production \
Copy link
Copy Markdown
Contributor

@pranavjain97 pranavjain97 Jun 16, 2025

Choose a reason for hiding this comment

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

why is this being hardcoded to production?

@mohammadalfaiyazbitgo
Copy link
Copy Markdown
Contributor Author

We should have different dockerfiles for both the apps to keep the dependencies limited to their app. Enclaved express should have a very light dockerfile.

Also, please lets add a docker-compose to ease up local dev, but you can do it in a follow-up.

They have the same dependencies, they only difference I can see is that they expose different ports

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds Docker and Podman support for both Master Express and Enclaved Express, including container build scripts, self-signed certificates, and updated health endpoint handling.

  • Introduce Dockerfile and .dockerignore for multi-stage container build
  • Add self-signed test and enclaved certificates into certs/ for mTLS
  • Update health router to map response.body.status and timestamp instead of raw body
  • Extend package.json scripts and README.md with Podman container instructions

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/masterBitgoExpress/routers/enclavedExpressHealth.ts Map enclaved response shape and update io-ts type
package.json Add container:build script to build Podman image
certs/test-ssl-key.pem Add self-signed private key for Master Express
certs/test-ssl-cert.pem Add self-signed certificate for Master Express
certs/enclaved-express-key.pem Add private key for Enclaved Express
certs/enclaved-express-cert.pem Add certificate for Enclaved Express
README.md Document Podman container build and run commands
Dockerfile Define multi-stage Dockerfile for build & production
.dockerignore Exclude unnecessary files from Docker build context
Comments suppressed due to low confidence (3)

README.md:180

  • [nitpick] When passing build arguments via Yarn scripts, you need to separate flags with --, e.g., yarn container:build -- --build-arg PORT=3080.
yarn container:build --build-arg PORT=3080

src/masterBitgoExpress/routers/enclavedExpressHealth.ts:14

  • [nitpick] Rather than leaving a TODO, consider extracting this common response type into a shared module now to avoid duplication and drift between services.
// TODO: Move to common definition between enclavedExpress and masterExpress

src/masterBitgoExpress/routers/enclavedExpressHealth.ts:81

  • [nitpick] The mapping from response.body to the new enclavedResponse shape hasn't been covered by tests. Please add or update tests to verify the status and timestamp are propagated correctly.
enclavedResponse: {

Comment thread Dockerfile Outdated
org.opencontainers.image.revision=${VCS_REF}

# Set runtime environment
ENV NODE_ENV={NODE_ENV} \
Copy link

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

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

The production stage uses ENV NODE_ENV={NODE_ENV} which will literally set the value to {NODE_ENV}. It should use ENV NODE_ENV=${NODE_ENV} to interpolate the build argument correctly.

Suggested change
ENV NODE_ENV={NODE_ENV} \
ENV NODE_ENV=${NODE_ENV} \

Copilot uses AI. Check for mistakes.
Comment thread Dockerfile
USER bitgo

# Expose port from build arg
EXPOSE ${PORT}
Copy link

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

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

The PORT build argument isn't declared or exported in the production stage, so EXPOSE ${PORT} may not work as expected. Consider adding ARG PORT and ENV PORT=${PORT} in the production stage.

Copilot uses AI. Check for mistakes.
Comment thread certs/test-ssl-key.pem
@@ -0,0 +1,28 @@
-----BEGIN PRIVATE KEY-----
Copy link

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

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

Committing private keys into the repository can lead to security risks. Consider generating these keys at build or runtime, or storing them in a secure secrets store instead of version control.

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,28 @@
-----BEGIN PRIVATE KEY-----
Copy link

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

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

This private key is also committed to version control. For security, keys should be generated or injected at runtime rather than stored in the repo.

Copilot uses AI. Check for mistakes.
@mohammadalfaiyazbitgo mohammadalfaiyazbitgo merged commit 95c4a3f into master Jun 16, 2025
3 checks passed
@mohammadalfaiyazbitgo mohammadalfaiyazbitgo deleted the WP-4681/dockerize-onprem branch June 16, 2025 18:07
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.

3 participants