add niks3 binary cache support #520
Conversation
WalkthroughAdds a new Niks3 integration NixOS module and imports it into the master module. The module provides options to enable Niks3, configure server URL, auth token file, and package; when enabled it wires systemd LoadCredential, adds the package to worker PATH, and appends a post-build niks3 upload step. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant BM as Buildbot Master (service)
participant W as Buildbot Worker (env PATH)
participant N3 as Niks3 Server
Dev->>BM: Trigger build
BM->>W: Schedule job / run build
W-->>BM: Produce artifact(s) (result-<attr>)
note over BM: postBuildSteps (niks3 enabled)
BM->>BM: systemd loads credential from cfg.niks3.authTokenFile
BM->>W: Ensure niks3 binary present in worker PATH (cfg.niks3.package)
BM->>N3: Run `niks3 push` (env: NIKS3_SERVER_URL, NIKS3_AUTH_TOKEN_FILE)
N3-->>BM: Acknowledgement / response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
nixosModules/niks3.nix (2)
17-21: Add URL format validation to serverUrl.The
serverUrloption accepts any string without validation. Consider adding format validation to catch configuration errors early.Apply this diff to add URL validation:
serverUrl = lib.mkOption { - type = lib.types.str; + type = lib.types.strMatching "https?://.*"; description = "niks3 server URL"; example = "https://niks3.yourdomain.com"; };
36-49: Verify postBuildSteps list merging.Similar to the
LoadCredentialconcern, directly assigning toservices.buildbot-nix.master.postBuildStepsrelies on NixOS list merging behavior. While this typically works for list-typed options, usinglib.mkAfterwould make the ordering and merging explicit.Consider using
lib.mkAfterfor clarity:- services.buildbot-nix.master.postBuildSteps = [ + services.buildbot-nix.master.postBuildSteps = lib.mkAfter [ { name = "Upload to niks3"; environment = { NIKS3_SERVER_URL = cfg.niks3.serverUrl; NIKS3_AUTH_TOKEN_FILE = interpolate "%(secret:niks3-auth-token)s"; }; command = [ "niks3" # note that this is the niks3 from the worker's $PATH "push" (interpolate "result-%(prop:attr)s") ]; } ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nixosModules/master.nix(1 hunks)nixosModules/niks3.nix(1 hunks)
🔇 Additional comments (3)
nixosModules/master.nix (1)
45-45: LGTM! Import follows the existing pattern.The addition of
./niks3.nixto the imports list is correctly placed and follows the same pattern as the existing./cachix.niximport.Verify that the niks3 module properly integrates with existing
LoadCredentialsettings in this file (lines 862-880) to avoid overriding existing credentials.nixosModules/niks3.nix (2)
1-12: LGTM! Module structure is correct.The module structure and
interpolatehelper function follow standard NixOS patterns and correctly create Buildbot interpolation objects.
32-34: LoadCredential lists merge automatically
Direct assignments toserviceConfig.LoadCredentialin separate modules are concatenated by NixOS’s list-merge strategy, solib.mkAfteris unnecessary. You can also removebuiltins.toStringaroundcfg.niks3.authTokenFile.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nixosModules/niks3.nix (1)
43-43: Addressed: ensuring niks3 on worker PATHAdding the package to systemd.services.buildbot-worker.path solves the earlier “binary must be present” concern.
🧹 Nitpick comments (1)
nixosModules/niks3.nix (1)
31-35: Package option: prefer mkPackageOption for discoverability and defaultsUse lib.mkPackageOption to standardize docs and defaults; keep existing fallback if needed.
- package = lib.mkOption { - type = lib.types.package; - default = pkgs.niks3 or (throw "niks3 package not found in pkgs. Please add niks3 flake input and overlay."); - description = "The niks3 package to use"; - }; + package = lib.mkPackageOption pkgs "niks3" { + default = (pkgs.niks3 or null); + # If pkgs.niks3 is absent, users can override this option explicitly. + # example = lib.literalExpression "inputs.niks3.packages.${pkgs.system}.niks3"; + description = "niks3 CLI package used on the worker PATH"; + };Please confirm mkPackageOption is available in your lib version. If not, we can keep types.package and improve description/defaultText instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nixosModules/master.nix(1 hunks)nixosModules/niks3.nix(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nixosModules/master.nix
🔇 Additional comments (1)
nixosModules/niks3.nix (1)
38-56: No changes to credential loading: loading the auth token viasystemd.services.buildbot-master.serviceConfig.LoadCredentialand passing it to workers withinterpolate "%(secret:niks3-auth-token)s"follows the existingcachixpattern and is correct.Likely an incorrect or invalid review comment.
Summary by CodeRabbit