carverauto/serviceradar

Fork 0

Update/proton ocaml driver bump #2252

Merged

mfreeman451 merged 2 commits from refs/pull/2252/head into main

2025-09-29 16:18:32 +00:00

mfreeman451 commented

2025-09-29 16:16:29 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1672
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1672
Original created: 2025-09-29T16:16:29Z
Original updated: 2025-09-29T16:18:36Z
Original head: carverauto/serviceradar:update/proton_ocaml_driver_bump
Original base: main
Original merged: 2025-09-29T16:18:32Z by @mfreeman451

PR Type

Enhancement

Description

Bump proton OCaml driver from 1.0.16 to 1.0.17
Remove vendored proton dependency files
Switch from local path pins to opam registry
Simplify Docker build configurations

Diagram Walkthrough

flowchart LR
  A["Vendored proton 1.0.16"] --> B["Registry proton 1.0.17"]
  C["Local path pins"] --> D["Opam registry pins"]
  E["Complex Docker setup"] --> F["Simplified Docker builds"]

File Walkthrough

Relevant files

Dependencies

MODULE.bazel `Update proton version in Bazel module` MODULE.bazel Update proton dependency version from 1.0.16 to 1.0.17	+1/-1
srql-translator.opam `Update proton version constraint in opam file` ocaml/srql-translator.opam Update proton dependency constraint from 1.0.16 to 1.0.17	+1/-1
opam `Remove vendored proton 1.0.16 opam package` third_party/vendor/tools_opam/extensions/opam/overrides_repo/packages/proton/proton.1.0.16/opam Delete entire vendored proton 1.0.16 opam package definition	+0/-47
url `Remove vendored proton 1.0.16 URL file` third_party/vendor/tools_opam/extensions/opam/overrides_repo/packages/proton/proton.1.0.16/url Delete vendored proton 1.0.16 URL configuration file	+0/-2

Configuration changes

Dockerfile.srql-builder `Simplify proton installation in builder Docker` docker/builders/Dockerfile.srql-builder Replace git repository pin with version-based pin Simplify proton installation using version number	+2/-3
Dockerfile.rpm.srql `Remove vendored proton setup in RPM Docker` docker/rpm/Dockerfile.rpm.srql Remove local proton-ocaml-driver copy operations Remove path-based opam pin for proton Update proton installation to use specific version	+1/-7
Dockerfile `Remove vendored proton setup in SRQL Docker` ocaml/srql/Dockerfile Remove local proton-ocaml-driver copy and git cleanup Remove path-based opam pin for proton Update proton installation to use specific version	+2/-8

Imported from GitHub pull request. Original GitHub pull request: #1672 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1672 Original created: 2025-09-29T16:16:29Z Original updated: 2025-09-29T16:18:36Z Original head: carverauto/serviceradar:update/proton_ocaml_driver_bump Original base: main Original merged: 2025-09-29T16:18:32Z by @mfreeman451 --- ### **PR Type** Enhancement ___ ### **Description** - Bump proton OCaml driver from 1.0.16 to 1.0.17 - Remove vendored proton dependency files - Switch from local path pins to opam registry - Simplify Docker build configurations ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Vendored proton 1.0.16"] --> B["Registry proton 1.0.17"] C["Local path pins"] --> D["Opam registry pins"] E["Complex Docker setup"] --> F["Simplified Docker builds"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Dependencies</strong></td><td><table> <tr> <td> <details> <summary><strong>MODULE.bazel</strong><dd><code>Update proton version in Bazel module</code>                                        </dd></summary> <hr> MODULE.bazel - Update proton dependency version from 1.0.16 to 1.0.17 </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1672/files#diff-6136fc12446089c3db7360e923203dd114b6a1466252e71667c6791c20fe6bdc">+1/-1</a>      </td> </tr> <tr> <td> <details> <summary><strong>srql-translator.opam</strong><dd><code>Update proton version constraint in opam file</code>                        </dd></summary> <hr> ocaml/srql-translator.opam - Update proton dependency constraint from 1.0.16 to 1.0.17 </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1672/files#diff-900d10104207edf7fbf525b51ccffa5711742688c181ed29a41cabd9ca1579c2">+1/-1</a>      </td> </tr> <tr> <td> <details> <summary><strong>opam</strong><dd><code>Remove vendored proton 1.0.16 opam package</code>                              </dd></summary> <hr> third_party/vendor/tools_opam/extensions/opam/overrides_repo/packages/proton/proton.1.0.16/opam - Delete entire vendored proton 1.0.16 opam package definition </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1672/files#diff-128716fdd4bd930d995a0e4b4d12c6173acfe608e0965b52f6bf3ec878eace9f">+0/-47</a>    </td> </tr> <tr> <td> <details> <summary><strong>url</strong><dd><code>Remove vendored proton 1.0.16 URL file</code>                                      </dd></summary> <hr> third_party/vendor/tools_opam/extensions/opam/overrides_repo/packages/proton/proton.1.0.16/url - Delete vendored proton 1.0.16 URL configuration file </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1672/files#diff-ffd958e53088d042c58263e517fa410be6d242b11a25614e6b3950452b17a1ed">+0/-2</a>      </td> </tr> </table></td></tr><tr><td><strong>Configuration changes</strong></td><td><table> <tr> <td> <details> <summary><strong>Dockerfile.srql-builder</strong><dd><code>Simplify proton installation in builder Docker</code>                      </dd></summary> <hr> docker/builders/Dockerfile.srql-builder <ul><li>Replace git repository pin with version-based pin<br> <li> Simplify proton installation using version number</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1672/files#diff-f10af98ad3045a3aeb6c2828c532daf6ad102b27ffe0da2111baa55be5e93ae8">+2/-3</a>      </td> </tr> <tr> <td> <details> <summary><strong>Dockerfile.rpm.srql</strong><dd><code>Remove vendored proton setup in RPM Docker</code>                              </dd></summary> <hr> docker/rpm/Dockerfile.rpm.srql <ul><li>Remove local proton-ocaml-driver copy operations<br> <li> Remove path-based opam pin for proton<br> <li> Update proton installation to use specific version</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1672/files#diff-4aed257fbbf9eddfb69e5c2c023f017af5ab13107d3221efc9ebfa288b9583e3">+1/-7</a>      </td> </tr> <tr> <td> <details> <summary><strong>Dockerfile</strong><dd><code>Remove vendored proton setup in SRQL Docker</code>                            </dd></summary> <hr> ocaml/srql/Dockerfile <ul><li>Remove local proton-ocaml-driver copy and git cleanup<br> <li> Remove path-based opam pin for proton<br> <li> Update proton installation to use specific version</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1672/files#diff-086d9103a276f32ea4c77dca8e9cd4a5b47c6cb4a73d1cbfd5f7d825361c19d1">+2/-8</a>      </td> </tr> </table></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-09-29 16:17:07 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1672#issuecomment-3347913478
Original created: 2025-09-29T16:17:07Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Version Consistency

Ensure all build and runtime environments consistently use proton 1.0.17 and that no remaining tooling still assumes 1.0.16 (including cached pins or transitive constraints).

"dune": "3.20.2",
"menhir": "20250903",
"yojson": "2.2.2",
"ppx_deriving": "6.0.3",
"lwt_ppx": "5.9.1",
"proton": "1.0.17",
"lwt": "5.9.2",
"tls": "2.0.2",
"tls-lwt": "2.0.2",
"mirage-crypto": "1.2.0",

Pin Strategy

Using opam pin with a plain version is unusual; consider opam install proton=1.0.17 to avoid creating a pin unless pinning is intentional.

ARG PROTON_VERSION="1.0.17"
RUN opam pin add proton "${PROTON_VERSION}" -y

ARG DREAM_REF="master"
RUN git clone --depth 1 --branch "${DREAM_REF}" https://github.com/aantron/dream.git /tmp/dream \
    && sed -i 's/"h2" {< "0.13.0"}/"h2" {>= "0.13.0" & < "0.14.0"}/' /tmp/dream/dream-httpaf.opam \

Lower Bound Check

Verify proton >= 1.0.17 is sufficient and that no upper bound is needed to prevent future breaking changes from newer major/minor releases.

depends: [
  "ocaml" {>= "4.14.0"}
  "dune" {>= "3.20"}
  "menhir"
  "yojson"
  "ppx_deriving" {>= "5.0"}
  "lwt_ppx"
  "proton" {>= "1.0.17"}
  "lwt"
  "tls" {>= "1.0.0"}
  "tls-lwt" {>= "1.0.0"}
  "mirage-crypto" {>= "1.2.0"}

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1672#issuecomment-3347913478 Original created: 2025-09-29T16:17:07Z --- ## PR Reviewer Guide 🔍 Here are some key observations to aid the review process: <table> <tr><td>⏱️ <strong>Estimated effort to review</strong>: 2 🔵🔵⚪⚪⚪</td></tr> <tr><td>🧪 <strong>No relevant tests</strong></td></tr> <tr><td>🔒 <strong>No security concerns identified</strong></td></tr> <tr><td>⚡ <strong>Recommended focus areas for review</strong><br><br> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1672/files#diff-6136fc12446089c3db7360e923203dd114b6a1466252e71667c6791c20fe6bdcR229-R238'><strong>Version Consistency</strong></a> Ensure all build and runtime environments consistently use proton 1.0.17 and that no remaining tooling still assumes 1.0.16 (including cached pins or transitive constraints). </summary> ```txt "dune": "3.20.2", "menhir": "20250903", "yojson": "2.2.2", "ppx_deriving": "6.0.3", "lwt_ppx": "5.9.1", "proton": "1.0.17", "lwt": "5.9.2", "tls": "2.0.2", "tls-lwt": "2.0.2", "mirage-crypto": "1.2.0", ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1672/files#diff-f10af98ad3045a3aeb6c2828c532daf6ad102b27ffe0da2111baa55be5e93ae8R28-R33'><strong>Pin Strategy</strong></a> Using opam pin with a plain version is unusual; consider opam install proton=1.0.17 to avoid creating a pin unless pinning is intentional. </summary> ```txt ARG PROTON_VERSION="1.0.17" RUN opam pin add proton "${PROTON_VERSION}" -y ARG DREAM_REF="master" RUN git clone --depth 1 --branch "${DREAM_REF}" https://github.com/aantron/dream.git /tmp/dream \ && sed -i 's/"h2" {< "0.13.0"}/"h2" {>= "0.13.0" & < "0.14.0"}/' /tmp/dream/dream-httpaf.opam \ ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1672/files#diff-900d10104207edf7fbf525b51ccffa5711742688c181ed29a41cabd9ca1579c2R12-R23'><strong>Lower Bound Check</strong></a> Verify proton >= 1.0.17 is sufficient and that no upper bound is needed to prevent future breaking changes from newer major/minor releases. </summary> ```txt depends: [ "ocaml" {>= "4.14.0"} "dune" {>= "3.20"} "menhir" "yojson" "ppx_deriving" {>= "5.0"} "lwt_ppx" "proton" {>= "1.0.17"} "lwt" "tls" {>= "1.0.0"} "tls-lwt" {>= "1.0.0"} "mirage-crypto" {>= "1.2.0"} ``` </details> </td></tr> </table>

qodo-code-review[bot] commented

2025-09-29 16:18:10 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1672#issuecomment-3347921276
Original created: 2025-09-29T16:18:10Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Install package directly instead of pinning Replace `opam pin add` with `opam install` to ensure the `proton` package is installed, not just pinned. docker/builders/Dockerfile.srql-builder [28-29] `ARG PROTON_VERSION="1.0.17" -RUN opam pin add proton "${PROTON_VERSION}" -y +RUN opam install "proton.${PROTON_VERSION}" -y` Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that `opam pin add <package> <version>` only pins the package without installing it, which would likely cause a build failure, and proposes a correct fix.	Medium
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1672#issuecomment-3347921276 Original created: 2025-09-29T16:18:10Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>General</td> <td> <details><summary>Install package directly instead of pinning</summary> ___ **Replace <code>opam pin add</code> with <code>opam install</code> to ensure the <code>proton</code> package is <br>installed, not just pinned.** [docker/builders/Dockerfile.srql-builder [28-29]](https://github.com/carverauto/serviceradar/pull/1672/files#diff-f10af98ad3045a3aeb6c2828c532daf6ad102b27ffe0da2111baa55be5e93ae8R28-R29) ```diff ARG PROTON_VERSION="1.0.17" -RUN opam pin add proton "${PROTON_VERSION}" -y +RUN opam install "proton.${PROTON_VERSION}" -y ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies that `opam pin add <package> <version>` only pins the package without installing it, which would likely cause a build failure, and proposes a correct fix. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>