Skip to content

build(docker): add DockerBuild target - #435

Merged
halcwb merged 5 commits into
informedica:masterfrom
7sharp9:Docker_consistency
Aug 4, 2026
Merged

build(docker): add DockerBuild target#435
halcwb merged 5 commits into
informedica:masterfrom
7sharp9:Docker_consistency

Conversation

@7sharp9

@7sharp9 7sharp9 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Add tracked FAKE targets for building and running the Docker image, including support for DOCKER_IMAGE/DOCKER_PLATFORM overrides and fail-fast checks for GENPRES_URL_ID and GENPRES_PASSWORD. Update the Dockerfile to copy Directory.Build.props during build and apply the shared app version, and refresh developer/agent docs to reflect the new Docker workflow.

Addresses point D1 in #423

Add tracked FAKE targets for building and running the Docker image, including support for DOCKER_IMAGE/DOCKER_PLATFORM overrides and fail-fast checks for GENPRES_URL_ID and GENPRES_PASSWORD. Update the Dockerfile to copy Directory.Build.props during build and apply the shared app version, and refresh developer/agent docs to reflect the new Docker workflow.
Copilot AI review requested due to automatic review settings August 3, 2026 16:53
@greptile-apps

greptile-apps Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds tracked FAKE targets for building and running the GenPRES Docker image, propagates the shared application version into image metadata, and updates contributor documentation.

  • Adds configurable DockerBuild and DockerRun targets with environment validation.
  • Makes the root Directory.Build.props available during containerized builds.
  • Labels produced images with the application version.
  • Documents image, platform, and credential configuration.

Confidence Score: 4/5

The credential disclosure on DockerRun failures should be fixed before merging.

DockerRun places both runtime secrets in the Docker client's argument list, while the shared failure handler renders that complete list into terminal or captured log output whenever Docker exits unsuccessfully.

Files Needing Attention: Build.fs and Helpers.fs

Security Review

DockerRun can disclose GENPRES_URL_ID and GENPRES_PASSWORD in plaintext when the Docker process fails because the shared process helper includes the full argument list in its exception.

Important Files Changed

Filename Overview
Build.fs Adds Docker build/run targets, but the run target exposes runtime credentials through the existing failure-message path.
Dockerfile Copies the shared version props into the build stage and adds an OCI version label with correct stage ordering.
DEVELOPMENT.md Replaces ad hoc Docker script guidance with documentation for the tracked FAKE targets and their overrides.
AGENTS.md Updates agent-facing Docker instructions to match the new target workflow.
.github/copilot-instructions.md Updates Copilot guidance with the new build/run commands and credential requirements.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Props[Directory.Build.props] --> Target[DockerBuild target]
  Target -->|APP_VERSION build arg| Build[docker build]
  Props -->|copied into workspace| Build
  Build --> Image[Version-labelled image]
  Env[Runtime environment] -->|GENPRES_URL_ID and GENPRES_PASSWORD| Run[DockerRun target]
  Image --> Run
  Run --> Container[GenPRES container on localhost:8080]
Loading

Reviews (1): Last reviewed commit: "build(docker): add DockerBuild target" | Re-trigger Greptile

Comment thread Build.fs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR standardizes the Docker build/run workflow in GenPRES by introducing tracked FAKE targets and ensuring Docker images carry the single curated app version from the repo’s root Directory.Build.props.

Changes:

  • Add DockerBuild and enhance DockerRun FAKE targets, with DOCKER_IMAGE / DOCKER_PLATFORM overrides and env-var fail-fast checks.
  • Update Docker image build to include root Directory.Build.props (to satisfy MSBuild imports) and to label images with org.opencontainers.image.version via APP_VERSION.
  • Refresh developer/agent documentation to reflect the new Docker workflow.

Reviewed changes

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

Show a summary per file
File Description
Dockerfile Copies Directory.Build.props into the build context and adds APP_VERSION/OCI version label for the runtime image.
Build.fs Introduces DockerBuild and updates DockerRun to validate env vars and run the tagged image with required settings.
DEVELOPMENT.md Documents the new DockerBuild/DockerRun workflow and cross-platform usage examples.
AGENTS.md Updates agent-facing Docker guidance to match the new FAKE targets and runtime expectations.
.github/copilot-instructions.md Mirrors the updated Docker guidance for Copilot usage in this repo.

Comment thread Dockerfile Outdated
Comment thread AGENTS.md Outdated
Comment thread .github/copilot-instructions.md Outdated
### Docker

The proprietary `GENPRES_URL_ID` is **not** baked into the image any more. Inject it (and `GENPRES_PASSWORD` for admin operations) at container runtime, ideally via a Docker / Kubernetes secret.
`GENPRES_URL_ID` is required at server startup, in both demo and production mode — not just for admin operations. The proprietary production value is **not** baked into the image; inject it (and `GENPRES_PASSWORD` for admin operations) at container runtime, ideally via a Docker / Kubernetes secret. For local testing without production credentials, use the public demo sheet ID documented in `.env.example`.
Comment thread DEVELOPMENT.md Outdated
Comment thread Build.fs
Update the DockerRun FAKE target to validate GENPRES_URL_ID and GENPRES_PASSWORD without embedding their values in docker CLI args. It now uses `-e VAR_NAME` passthrough instead of `-e VAR_NAME=value`, preventing secret exposure in process error output while still failing fast when variables are missing.
Copilot AI review requested due to automatic review settings August 3, 2026 17:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Dockerfile:46

  • The comment for APP_VERSION still points readers to the old docker-local.sh / docker-amd64.sh templates, but DEVELOPMENT.md no longer contains those scripts after this change. This makes the guidance stale and harder to follow.
# Curated single version number for the whole app (server, client, libraries).
# Sourced from the root Directory.Build.props by the caller (see docker-local.sh /
# docker-amd64.sh templates in DEVELOPMENT.md) so the image label always matches
# what was actually built, without duplicating the version here.

AGENTS.md:81

  • This paragraph implies GENPRES_PASSWORD is only needed for admin operations, but the Docker image defaults GENPRES_PROD=1 (Dockerfile), and Server.fs refuses to start in production mode unless GENPRES_PASSWORD is set and strong enough (see .env.example). The wording should reflect that Docker runs require a password unless you explicitly run with GENPRES_PROD=0.
`GENPRES_URL_ID` is required at server startup, in both demo and production mode — not just for admin operations. The proprietary production value is **not** baked into the image; inject it (and `GENPRES_PASSWORD` for admin operations) at container runtime, ideally via a Docker / Kubernetes secret. For local testing without production credentials, use the public demo sheet ID documented in `.env.example`.

.github/copilot-instructions.md:81

  • This paragraph implies GENPRES_PASSWORD is only needed for admin operations, but the Docker image defaults GENPRES_PROD=1 (Dockerfile), and Server.fs refuses to start in production mode unless GENPRES_PASSWORD is set and strong enough (see .env.example). The wording should reflect that Docker runs require a password unless you explicitly run with GENPRES_PROD=0.
`GENPRES_URL_ID` is required at server startup, in both demo and production mode — not just for admin operations. The proprietary production value is **not** baked into the image; inject it (and `GENPRES_PASSWORD` for admin operations) at container runtime, ideally via a Docker / Kubernetes secret. For local testing without production credentials, use the public demo sheet ID documented in `.env.example`.

Update the Dockerfile comment to reference the `DockerBuild` FAKE target in `Build.fs` and `DEVELOPMENT.md` as the source of `APP_VERSION`. This keeps the documentation aligned with the current Docker build workflow.
Copilot AI review requested due to automatic review settings August 3, 2026 17:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Build.fs:284

  • requireEnvVar treats empty strings as unset, but it will accept whitespace-only values (e.g. GENPRES_PASSWORD=" "). The server treats whitespace-only as not-set (String.IsNullOrWhiteSpace), so this can bypass the intended fail-fast check and then fail later inside the container with a less direct error. Align the check with server-side semantics by using IsNullOrWhiteSpace here too.
let requireEnvVar name =
    match System.Environment.GetEnvironmentVariable name with
    | null
    | "" -> failwithf "%s is not set. Load it from .env first (see DEVELOPMENT.md)." name
    | v -> v

Update `requireEnvVar` in `Build.fs` to treat whitespace-only values as missing, so `DockerRun` fails fast with a clear error instead of accepting invalid env values.

Also update Docker documentation in `AGENTS.md`, `.github/copilot-instructions.md`, and `DEVELOPMENT.md` to clarify that the image defaults to `GENPRES_PROD=1`, so both `GENPRES_URL_ID` and a 16+ character `GENPRES_PASSWORD` are required at runtime.
Copilot AI review requested due to automatic review settings August 3, 2026 17:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@halcwb

halcwb commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

@7sharp9 Is it possible to also set the user name as an env variable, i.e. GENPRES_USERNAME, so it doesn't default to my specific username?

@7sharp9

7sharp9 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

@7sharp9 Is it possible to also set the user name as an env variable, i.e. GENPRES_USERNAME, so it doesn't default to my specific username?

DOCKER_IMAGE is really an alias for the fully qualified name now, we could add GENPRES_USERNAME:

//   1. DOCKER_IMAGE       - full override, e.g. a different fully qualified name/repo entirely
//   2. GENPRES_USERNAME   - your own Docker Hub namespace, image stays "genpres"
//   3. "halcwb/genpres"   - project default

For me, that feels incomplete, we could switch to requiring GENPRES_USERNAME/GENPRES_APPNAME which would be more descripting of their domain usage. and default to halcwb/genpres is those are not set.

@halcwb

halcwb commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

@7sharp9 Is it possible to also set the user name as an env variable, i.e. GENPRES_USERNAME, so it doesn't default to my specific username?

DOCKER_IMAGE is really an alias for the fully qualified name now, we could add GENPRES_USERNAME:

//   1. DOCKER_IMAGE       - full override, e.g. a different fully qualified name/repo entirely
//   2. GENPRES_USERNAME   - your own Docker Hub namespace, image stays "genpres"
//   3. "halcwb/genpres"   - project default

For me, that feels incomplete, we could switch to requiring GENPRES_USERNAME/GENPRES_APPNAME which would be more descripting of their domain usage. and default to halcwb/genpres is those are not set.

I got it, let's leave it for now as it is. Thanks.

@halcwb
halcwb merged commit 13a670a into informedica:master Aug 4, 2026
4 checks passed
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