Skip to content

Conversation

@djeebus
Copy link
Contributor

@djeebus djeebus commented Nov 6, 2025

  • Add new env vars
  • Add local dev docs

Note

Adds local template build workflow and logs collector to local dev, and refactors orchestrator/storage to use configurable absolute cache paths with startup dir creation.

  • Local Dev:
    • Update DEV-LOCAL.md with end-to-end steps to build/run locally (kernels/envd/fc versions, base template build).
    • Add packages/local-dev/vector.toml and vector service; add Loki healthcheck in docker-compose.yaml.
    • Add .env.local vars for api, client-proxy, and orchestrator (incl. LOGS_COLLECTOR_ADDRESS, local cluster, cache dirs).
    • Makefile: add download-public-kernels; add shared/scripts target local-build-base-template.
  • Orchestrator:
    • Introduce expanded, absolute path settings in BuilderConfig (DefaultCacheDir, SandboxCacheDir, SnapshotCacheDir, TemplateCacheDir, TemplatesDir, etc.) and ensure directories exist at startup.
    • Plumb BuilderConfig through builder/template/sandbox code; remove reliance on global base path.
    • Update sandbox FC/startup, pause/resume, and cleanup to use config-driven paths; route cache dirs into diff/snapshot creation.
    • Template cache/store now use config for local cache and NFS cache; delete old cache on start.
    • build-template cmd and benchmarks updated to parse/pass builder config.
  • Shared Storage:
    • Refactor storage helpers to accept a BuilderConfig interface for cache/snapshot/template paths; remove shared/pkg/config.go.
  • Misc:
    • .gitignore: ignore /packages/fc-kernels.
    • Orchestrator Makefile: simplify run-local (no explicit port).

Written by Cursor Bugbot for commit 75950b7. This will update automatically on new commits. Configure here.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@claude
Copy link

claude bot commented Nov 8, 2025

Pull Request Review: Support template creation in local dev

Summary

This PR refactors the orchestrator configuration to support local template creation with better configuration management and absolute path handling.

✅ Strengths

  1. Excellent Documentation Updates - DEV-LOCAL.md improvements are comprehensive with clear step-by-step setup
  2. Configuration Refactoring - Moving from implicit global paths to config-driven architecture is solid
  3. Path Safety Improvements - makePathsAbsolute() systematically ensures all paths are absolute

🔴 Critical Issues

1. Missing Directory Creation (High Priority)

The PR description mentions 'Create dirs at startup (ensureDirs)' but this implementation is not in the diff. Cache directories may not exist at startup.

Recommendation: Add EnsureDirs() method and call it after Parse()/ParseBuilder()

2. Inconsistent Naming Convention (Medium)

Mixed use of *Path and *Dir suffixes. Consider standardizing on one convention.

3. Breaking Change Documentation (High)

Removing global OrchestratorBasePath - ensure migration path is documented for existing deployments.

🟡 Moderate Concerns

1. Test Coverage Gaps (High Priority)

Missing tests for:

  • ParseBuilder() function
  • Getter methods (GetSandboxCacheDir, GetSnapshotCacheDir, GetTemplateCacheDir)
  • makePathsAbsolute() error cases
  • cleanupFiles() function

2. Environment Variable Defaults (Medium)

SnapshotCacheDir defaults to /mnt/snapshot-cache which requires root permissions and may not exist in all environments. Consider fallback for local dev.

3. Missing .env.local Validation

Add startup validation for required vs optional variables with clear error messages.

🟢 Minor Issues

  • Typo: 'absolutify' in error message (line 62) - consider 'resolve absolute path'
  • Gitignore: Also add /packages/orchestrator/tmp/
  • Empty string handling could warn for empty critical paths

🔒 Security

  • ✅ Directory Permissions appropriate
  • ✅ Path Traversal protection via filepath.Join/Abs
  • ⚠️ Verify utils.SymlinkForce() validates targets

🧪 Testing Priority 1 (Block merge)

  1. Add unit tests for ParseBuilder()
  2. Implement/verify ensureDirs() functionality
  3. Test path resolution edge cases

📝 Documentation Needs

  1. Migration guide for old OrchestratorBasePath usage
  2. Architecture Decision Record
  3. Troubleshooting section in DEV-LOCAL.md

✅ Fix Before Merge

  1. Add unit tests for new public functions
  2. Implement directory creation at startup
  3. Document /mnt/snapshot-cache requirement or provide fallback
  4. Add env variable validation with clear errors

🎯 Overall Assessment

Code Quality: 8/10
Completeness: 7/10
Risk Level: Medium

Recommendation:Approve with changes requested

The PR is fundamentally sound with valuable improvements. Main blockers are missing directory creation, test coverage, and documentation of requirements.

🙏 Positive Notes

  • Comprehensive documentation updates
  • Careful systematic refactoring across 28 files
  • Config-driven architecture is best practice
  • Excellent detailed PR description

Great work on this substantial refactoring! 🚀

assert.Equal(t, []string{"service1", "service2"}, config.Services)
})

t.Run("env defaults get defaults before expansion", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the test!

Copy link
Member

@jakubno jakubno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀 leaving the decision to you

@jakubno jakubno assigned jakubno and unassigned ValentaTomas Nov 13, 2025
@djeebus djeebus merged commit 04a6e44 into main Nov 14, 2025
28 checks passed
@djeebus djeebus deleted the cleaner-local-dev branch November 14, 2025 00:49
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.

4 participants