Skip to content

Conversation

@skrawcz
Copy link
Contributor

@skrawcz skrawcz commented Apr 3, 2025

We missed exposing this in the base docker file for the frontend such that it was missed when the docker
file was built.

Changes

  • wires through the REACT_APP_HAMILTON_SUB_PATH var

How I tested this

  • locally by building and changing the value to /hamilton3 and then serving the UI from that subpath.

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

We missed exposing this in the base docker file for the
frontend such that it was missed when the docker
file was built.
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Reviewed everything up to 974af44 in 1 minute and 7 seconds

More details
  • Looked at 48 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. ui/docker-compose-prod.yml:63
  • Draft comment:
    Consider adding a comment explaining when and why the REACT_APP_HAMILTON_SUB_PATH should be set vs left empty.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
2. ui/docker-compose.yml:64
  • Draft comment:
    Add documentation if non-empty subpaths require specific configurations to ensure proper routing.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
3. ui/frontend/Dockerfile.frontend:24
  • Draft comment:
    Ensure that the build arg or ENV variable REACT_APP_HAMILTON_SUB_PATH is documented in the project docs, as it changes the frontend configuration.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
4. ui/frontend/Dockerfile.frontend-prod:26
  • Draft comment:
    Docstring or added comment might help future developers understand the purpose of REACT_APP_HAMILTON_SUB_PATH in the build process.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
5. ui/docker-compose-prod.yml:63
  • Draft comment:
    Added REACT_APP_HAMILTON_SUB_PATH with an empty default. Consider documenting its intended use in the deployment docs and verifying that an empty string is acceptable for subpath routing.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
6. ui/docker-compose.yml:64
  • Draft comment:
    Ensure REACT_APP_HAMILTON_SUB_PATH is documented in the Docker deployment instructions. The empty default should be explicitly understood by users.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
7. ui/frontend/Dockerfile.frontend:24
  • Draft comment:
    For consistency with other build-time variables, consider adding an ARG for REACT_APP_HAMILTON_SUB_PATH. If runtime injection is intended only, a comment explaining this choice would help future maintainers.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
8. ui/frontend/Dockerfile.frontend-prod:26
  • Draft comment:
    Consider declaring an ARG for REACT_APP_HAMILTON_SUB_PATH in the build stage (as done for REACT_APP_AUTH_MODE et al.) if build-time customization is desired, or add a comment explaining why it's injected only at runtime.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_9TE9m9NsKdmebmGn


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@elijahbenizzy elijahbenizzy merged commit fa85f36 into main Apr 7, 2025
24 of 25 checks passed
@elijahbenizzy elijahbenizzy deleted the fix_docker_subpath branch April 7, 2025 02:45
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.

2 participants