openapi-gen-build #10
No reviewers
Labels
No labels
automerge
bug
dependencies
duplicate
enhancement
help wanted
invalid
migrations
question
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Reference
Personal/woodpecker-plugins!10
Loading…
Reference in a new issue
No description provided.
Delete branch "openapi-gen-build"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Review Summary
This PR migrates the workspace from naersk to crane, replaces the generated forgejo-client crate with an
api-clientsworkspace member that uses openapi-generator-cli at build time, and updates CI images.Critical:
cargo buildwithout--workspacein virtual workspaceThe root
Cargo.tomlis a virtual workspace (only[workspace], no[package]). Runningcargo buildwithout-por--workspacewill fail with an error. The old command wascargo build --workspace --all-targets --locked.Security: Unverified download
openapi-generator-cli.jaris fetched from Maven with no checksum verification. A MITM or supply-chain compromise could substitute a malicious JAR.CI inconsistency: Build and Test steps use different Rust versions
Build pins
rust:1.97-bookworm; Test usesrust:1-bookworm(latest stable). If build scripts re-run duringcargo test, openapi-generator-cli won't be available in the Test step's container.Minor: Stale doc comment in
core/src/forgejo.rs:72References
forgejo_client::apis::configuration::Configurationwhich no longer exists.*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: changes
Code review findings from opencode.
@ -25,1 +16,3 @@- cargo build --workspace --all-targets --locked- version=7.23.0- apt-get update -qq && apt-get install -y -qq default-jre-headless- curl -fsSL --retry 3 https://repo1.maven.org/maven2/org/openapitools/openapi-generator-cli/$version/openapi-generator-cli-$version.jar -o /usr/local/bin/openapi-generator-cli.jar[security] JAR downloaded without checksum verification. Add
echo '<sha256> <file>' | sha256sum -cor use a pinned Nix derivation to verify integrity.@ -26,2 +21,4 @@- chmod +x /usr/local/bin/openapi-generator-cli- cargo build- name: Test[bug] Test step does not install openapi-generator-cli. If cargo re-runs
api-clients/build.rs(which shells out toopenapi-generator-cli), the step will fail. Either install it here too or move codegen to its own step persisted via shared workspace.[style] Test step uses
rust:1-bookworm(latest) while Build pinsrust:1.97-bookworm. Pin both to the same version for reproducible CI.@ -26,0 +19,4 @@- test -s /usr/local/bin/openapi-generator-cli.jar- printf '#!/bin/sh\nexec java -jar /usr/local/bin/openapi-generator-cli.jar "$@"\n' > /usr/local/bin/openapi-generator-cli- chmod +x /usr/local/bin/openapi-generator-cli- cargo build[bug]
cargo buildwithout--workspacewill fail on a virtual workspace (root Cargo.toml has only[workspace], no[package]). Restore--workspace(and--lockedto match the requirement in AGENTS.md).[bug]
--lockedwas removed. AGENTS.md states CI runs--lockedon every cargo command to detect stale Cargo.lock. Without it, deps can silently drift.@ -71,10 +72,7 @@ pub fn resolve_forgejo_token(settings_forgejo_token: Option<&str>) -> Result<Str/// Build a [`forgejo_client::apis::configuration::Configuration`] from a base URL[style] Doc comment still references
forgejo_client::apis::configuration::Configuration(the old crate). Update toapi_clients::forgejo::apis::configuration::Configurationor justConfigurationsince it's imported.Review Summary
This PR migrates from naersk to crane, replaces the pre-generated forgejo-client crate with an
api-clientscrate that generates its code at build time viaopenapi-gen-build, and switches from native-tls to rustls. The overall direction is good, but there are two issues that would prevent correct evaluation/build and one maintainability concern.Needs fixing
forgejo-client-genstale reference —flake.nixstill tries to exposeforgejo-client-gen = pluginScope.forgejo-client-genin thepackagesset (line 158), butgenForgejoClientand itspluginScopeentry were deleted. This will cause an evaluation error if anyone runsnix build .#forgejo-client-genor lists flake outputs.DevShell missing build dependencies — The
api-clientsbuild script requiresopenapi-generator-cli+jreat build time, but thenix developshell does not provide them. Runningcargo build -p forgejo-opencode-reviewin the dev shell will panic with "openapi-generator-cli is not installed".Concern
.woodpecker/verify.yamlhardcodes version7.23.0while nix builds use whatever nixpkgs ships. Different generator versions can produce different rust code, leading to subtle CI-vs-image inconsistencies.*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: changes
Code review findings from opencode.
@ -23,2 +14,3 @@depends_on: Generate codeimage: docker.io/rust:1.97-bookwormcommands:- version=7.23.0[style] openapi-generator-cli version
7.23.0is hardcoded here but comes from nixpkgs in nix builds. A version mismatch can produce different generated rust code. Consider pulling the version from a single source (e.g., a file nix and the CI script both read).@ -163,3 +161,1 @@devShells.default = pkgs.mkShell {buildInputs = with pkgs; [rustToolchaindevShells.default = rustPlatform.devShell {[bug] DevShell must include
openapi-generator-cliandjre(orjdk_headless). Theapi-clients/build.rspanics if the CLI isn't found, socargo build -p forgejo-opencode-reviewfails innix develop.@ -160,9 +158,8 @@forgejo-client-gen = pluginScope.forgejo-client-gen;[bug] Stale reference:
forgejo-client-gen = pluginScope.forgejo-client-gen;butgenForgejoClientwas removed from the scope. This will fail at evaluation for.#forgejo-client-genornix flake show. Remove this line.Review Summary
This PR migrates the Nix build system from naersk to crane, replaces the pre-generated
forgejo-clientcrate with a build-script-generatedapi-clientscrate, simplifies the CI pipeline, and updates various configuration files accordingly.The structural changes are sound and follow workspace conventions. The CI now installs
openapi-generator-clidirectly via curl instead of relying on Nix for codegen, and all verify steps run in a single container.Key concerns:
#[path]inapi-clients/src/lib.rspoints outside the crate directory, which is fragile and may confuse tooling.*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: changes
Code review findings from opencode.
[style] Combining
cargo build,cargo test, andclippyinto a single step loses per-stage failure visibility in the Woodpecker UI. If CI cost is a concern, consider keeping them as separate steps withdepends_onto preserve operational clarity while maintaining the same overall execution order.[security] openapi-generator-cli JAR is downloaded without checksum verification (
curl -fsSL --retry 3 ...with onlytest -sas validation). The previous Nix pipeline implicitly verified hashes via nixpkgs. Add a checksum check (e.g. download a.sha256or.ascalongside the JAR, or hardcode the expected SHA-256) to prevent supply-chain attacks via MITM.@ -0,0 +1,6 @@#![allow(unused_imports)]#[path = "../../target/generated/api-clients/generated/mod.rs"]pub mod generated;[style]
#[path = "../../target/generated/api-clients/generated/mod.rs"]points outside the crate's directory tree. This ties the crate to a workspace-leveltarget/layout andrust-analyzer/IDE tooling may fail to resolve the module. Consider using ainclude!macro or generating the file underapi-clients/src/instead.