-
Notifications
You must be signed in to change notification settings - Fork 0
Update flowctl process and cleanup #19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request enhances flowctl's pipeline orchestration capabilities by adding comprehensive run tracking and management features while removing broken/incomplete CLI commands. The changes introduce a new pipeline run tracking API, update protobuf definitions, and significantly clean up documentation.
Key Changes
- Added pipeline run tracking infrastructure (storage, control plane RPCs, runner integration)
- Regenerated protobuf files with version updates (protoc-gen-go-grpc v1.5.1 → v1.6.0, protoc v6.33.0 → v6.32.1)
- Removed incomplete CLI commands (
apply,new,context,help) with migration guidance - Deleted outdated example pipeline files and pitch documentation
- Added comprehensive getting-started guide
Reviewed changes
Copilot reviewed 80 out of 82 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| proto/*.pb.go | Regenerated protobuf files with updated tool versions and spelling corrections |
| proto/control_plane.proto | Added pipeline run management RPCs (Create, Update, Get, List, Stop) |
| internal/storage/*.go | Added pipeline run storage methods to both memory and BoltDB implementations |
| internal/api/control_plane.go | Implemented pipeline run management RPC handlers |
| internal/runner/pipeline_runner.go | Integrated run tracking with pipeline lifecycle |
| internal/runner/loader.go | Added component resolution and registry support |
| internal/components/*.go | New component resolver and translator for registry-based components |
| examples/ | Removed outdated quickstart examples, added comprehensive getting-started guide |
| pitches/ | Removed all pitch documentation files |
| cmd/root.go | Removed apply and quickstart commands, added processors command |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this 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
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Register implements flowctlpb.ControlPlane.Register | ||
| func (w *ControlPlaneWrapper) Register(ctx context.Context, req *flowctlpb.ServiceInfo) (*flowctlpb.RegistrationAck, error) { | ||
| v1Req := &flowctlv1.RegisterRequest{ | ||
| Component: &flowctlv1.ComponentInfo{ | ||
| Id: req.ComponentId, | ||
| Type: convertToV1ComponentType(req.ServiceType), | ||
| }, | ||
| } | ||
|
|
||
| _, err := w.server.RegisterComponent(ctx, v1Req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &flowctlpb.RegistrationAck{ | ||
| ServiceId: req.ServiceId, | ||
| }, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return generated service ID from wrapper register
ControlPlaneWrapper.Register builds a v1 request without propagating the incoming service_id and then returns the original request ID instead of the ID assigned by RegisterComponent (internal/api/control_plane.go:648-664). Flowctlpb clients (e.g., RegisterPipeline) will therefore keep using their requested ID, while the control plane stores the service under the generated ID; subsequent heartbeats are looked up by the service_id sent by the client, so Heartbeat immediately fails with “service not found” (see lines 262-265), preventing any service registered through the wrapper from reporting health.
Useful? React with 👍 / 👎.
This pull request introduces significant documentation improvements and major breaking changes to the CLI by removing several commands. The documentation now provides clearer explanations of the system architecture, configuration, troubleshooting, and available commands. Additionally, the
flowctl apply,flowctl new, andflowctl contextcommands have been removed, and users are guided toward their replacements. This reduces user confusion and simplifies maintenance.CLI Breaking Changes and Migration:
flowctl applycommand (useflowctl translateinstead),flowctl new(useflowctl init), andflowctl context(feature was not implemented); also removed the redundantflowctl helpcommand—use the--helpflag instead. Migration instructions and rationale are provided inCHANGELOG.md.cmd/apply/apply.go,cmd/apply/process.go, andcmd/context.go. [1] [2] [3]Documentation and Usability Improvements:
README.mdwith a new architecture overview, updated YAML configuration examples, clearer explanation of component roles, and a detailed troubleshooting section covering common issues and solutions. [1] [2] [3]README.mdwith links to guides, advanced topics, and real-world examples.CLAUDE.mdto reflect the new command structure and provide a categorized list of available commands, replacing references to removed commands and improving clarity. [1] [2]