Skip to content

Review the port of diamond-tiled into dls-tiled #15

Closed

Description

@gilesknap

@ZohebShaikh dls-tiled is intended to be the CI-built, auto-published home for everything in diamond-tiled. We have ported your work across. Could you review it and confirm that nothing is missing or changed in a way you don't want? Once you're happy, we'll archive diamond-tiled.

What dls-tiled is: the upstream ghcr.io/bluesky/tiled release image (pinned via TILED_VERSION in the Dockerfile, currently 0.2.18) with the DLS plugins installed into its venv as the dls_tiled package. Each git tag publishes:

  • the image ghcr.io/diamondlightsource/dls-tiled:<tag>
  • the Helm chart oci://ghcr.io/diamondlightsource/charts/tiled (version and appVersion = tag)

Both are public. CI publishes the chart only after the image has been pushed.

Ported from diamond-tiled:

  • Helm chart: helm/tiled → Charts/tiled (Add generic tiled Helm chart and publish it from CI #8, Publish the chart only when the container image was pushed #12). Changes: the default image is ghcr.io/diamondlightsource/dls-tiled (diamond-tiled's chart defaulted to upstream ghcr.io/bluesky/tiled); pullPolicy: IfNotPresent; the TILED_SINGLE_USER_API_KEY secretKeyRef is optional: true, so the chart installs without the secret.
  • DLS OPA access policy: diamond_policy/dls.py → src/dls_tiled/policy.py (Add the DLS OPA access policy from diamond-tiled #14). The logic is unchanged, and 29 tests run against a mocked OPA. Because it is now part of the installed package, a ConfigMap mounted at /deploy/config can no longer hide it (in diamond-tiled it was copied into /deploy/config, which the chart's ConfigMap mount covers). It is enabled with access_control: {access_policy: dls_tiled.policy:DiamondOpenPolicyAgentAuthorizationPolicy, args: ...}; see the README and the commented example in Charts/tiled/values.yaml. Questions from the review:
    • Do the OPA endpoint names (tiled/user_session(s), tiled/scopes, tiled/modify_session) and result shapes match the live DLS OPA bundle? The README example uses placeholder URL and audience values. What should they be?
    • Is empty_access_blob_public: true the intended default? It applies only when a client sends access_blob: null; tiled clients send {} by default.
    • allowed_scopes (inherited from tiled's base class) does not call _check_principal, so anonymous requests reach OPA with no token. Should it return 401 like the other methods?
    • A malformed access tag stored on a node gives a 500 on reads. Should build_input deny instead?
    • Should modify_node also send OPA the node's current blob, so OPA can check access to the old value?
  • Envoy per-beamline routing: diamond-tiled#2 → Add envoy proxy for per-beamline ingress routing #11 (open). See the review comment there. The 15 s route timeout, the missing websocket upgrade and the open admin port are the blockers. The per-beamline config, storage and auth question decides the overall shape for a central DLS tiled.
  • Data restructure scripts and docs: diamond-tiled#1 → Add tiled data restructure scripts and docs #10 (open).

Not from diamond-tiled, for context: dls_tiled.visr (the ViSR /binned router, opt-in via config.routers), with #6 open from Hiran; and a shipped /deploy/log_config.yml so the plugin loggers are visible.

Housekeeping: you will need write access to merge. Giles is arranging that.

Activity

  1. fajinyuan commented on Oct 20, 2026

    @fajinyuan
    Contributor

    @ZohebShaikh could you work and comment on this request please ? I don't understand most of these. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions