feat(e2e): add the workload conformance recorder package - #220
feat(e2e): add the workload conformance recorder package#220AviadHayumi wants to merge 15 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds a Go end-to-end module with a recorder package. The recorder models workload journeys, watches Kubernetes state transitions, executes resume and scale actions, validates observed order, and writes versioned YAML recordings. ChangesEnd-to-end recorder
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Flow
participant Kubernetes
participant Recorder
participant Recording
Flow->>Kubernetes: create workload from manifest
Kubernetes-->>Recorder: workload watch events
Recorder->>Recorder: classify and settle resource states
Recorder->>Kubernetes: apply resume or scale merge patch
Recorder->>Recording: append STATE and ACTION events
Recorder->>Recording: validate journey and write YAML
Flow->>Kubernetes: delete workload
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (7)
test/e2e/recorder/recorder.go (4)
313-318: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExtend the dedup drop list to cover per-sync timestamps.
significantCRdropsmetadata.resourceVersionandmetadata.managedFields. Many controllers also re-stamp timestamps on every sync, for examplestatus.conditions[].lastTransitionTime,lastUpdateTime, andlastHeartbeatTime. Those fields change without a state change, sokeeptreats each resync as a distinct CR.The order check still passes, because
ObservedOrderErrcompacts consecutive repeats atorder.goline 21. The cost is a recorded fixture that grows with every resync, and a longer event stream for the replay tests to walk. Decide the drop list when you add the first real flows, since the exact fields depend on the operators.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/recorder/recorder.go` around lines 313 - 318, Extend significantCR to remove controller-maintained per-sync timestamp fields, including status.conditions[].lastTransitionTime, lastUpdateTime, and lastHeartbeatTime, alongside the existing metadata fields. Define the drop list based on the timestamp fields used by the actual flows being added, while preserving the existing significantCR return behavior.
39-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider holding the cluster dependencies on
Recorderinstead of package globals.
Bindwrites five package-level variables that every flow then reads. Two consequences follow. First, flows cannot run in parallel or against different clients, because all of them share one global set. Second,Bindtakesversionandnsas adjacent string parameters, so a caller can transpose them without a compile error.Storing the clients, namespace, version, and progress writer on
Recorderwould remove both problems. The API surface is new in this PR, so the change is cheapest now.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/recorder/recorder.go` around lines 39 - 54, Refactor the recorder flow to store cluster dependencies on a Recorder instance rather than the package-level k8sClient, dynClient, serverVersion, namespace, and progress variables. Update Bind to initialize and return or expose a Recorder containing named dependency fields, including distinct namespace and version fields, then update all flows and methods to read those fields through the instance so parallel or differently configured recorders do not share state.
286-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve error context consistently across the recorder package. Three sites drop the error chain, so a caller cannot inspect the underlying cause with
errors.Isorerrors.As. The shared root cause is inconsistent application of the%wwrapping rule; the package already wraps correctly atrecorder.golines 65, 69, 217, 227, 259, 264, and 378.
test/e2e/recorder/recorder.go#L286-L286: changefailure stringtofailure error, assign the error directly at lines 115, 134, and 175, build the timeout messages withfmt.Errorf, and returnrec.failureat line 83 instead oferrors.New(rec.failure).test/e2e/recorder/recorder.go#L235-L242: wrap thek8sClient.Geterror at line 239 with%wand name the object, matching line 227.test/e2e/recorder/recording.go#L73-L94: wrap theos.MkdirAllerror at line 75, theyaml.Marshalerror at line 79, and theos.ReadFileerror at line 88, matching the path context already added at line 91.As per coding guidelines: "Wrap errors with
%wverb in Go error handling".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/recorder/recorder.go` at line 286, Preserve wrapped error chains across the recorder package: in test/e2e/recorder/recorder.go:286, change failure to error, assign errors directly at lines 115, 134, and 175, create timeout failures with fmt.Errorf, and have the recorder return failure at line 83 without errors.New; at test/e2e/recorder/recorder.go:235-242, wrap k8sClient.Get with %w and include the object name; at test/e2e/recorder/recording.go:73-94, wrap os.MkdirAll, yaml.Marshal, and os.ReadFile errors with %w while retaining path context.Source: Coding guidelines
184-187: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFilter watch events by type instead of relying on object-type assertion.
The dynamic client decodes watch events into
*unstructured.Unstructured, includingBookmarkevents. When a bookmark is received, itsevent.Objectcontains onlymetadata.resourceVersion. The type assertion at line 184 would not reject it.If a bookmark is processed,
Classify()returns empty, setting state toUndefinedStatus. ThenstatusSettled()returnstruebecause generation fields are absent, andrec.keep()records a spurious state inrec.order. This causes the order check to fail.Currently,
AllowWatchBookmarksis never set inwatchWorkload()(lines 244-249), so bookmarks are not sent by the API server and this path does not execute. Add an explicit event-type check to keep the behavior correct if bookmarks are enabled later.♻️ Proposed event-type filter
+ if event.Type != watch.Added && event.Type != watch.Modified { + continue // bookmarks and other non-data events carry no state to record + } u, ok := event.Object.(*unstructured.Unstructured) if !ok { - continue // a bookmark carries no workload object + continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/recorder/recorder.go` around lines 184 - 187, Update the watch-event handling in watchWorkload to filter on the event type before processing the object, explicitly skipping watch.Bookmark events rather than relying on the *unstructured.Unstructured assertion. Preserve processing for normal workload events and keep the existing malformed-object guard for non-bookmark events.test/e2e/recorder/recorder_internal_test.go (1)
111-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for an empty observed sequence.
ObservedOrderErrreturns "no states observed" when the compacted sequence is empty (order.go lines 22-24). No table case covers that branch. A flow that records nothing reaches this path throughFlow.Run, so the branch is worth a case.Note that
terminal(c.journey)requires a non-empty journey, so passwantexplicitly for this case or keep a declared journey with empty observations.♻️ Proposed table case
{"wrong terminal", steps(initializing, running, completed), []kartav1alpha1.ResourceStatus{initializing, running}, false}, + {"no states observed", steps(initializing, running, completed), nil, false},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/recorder/recorder_internal_test.go` around lines 111 - 126, Add a table-driven test case in the existing tests table covering an empty observed status sequence, ensuring the expected result and error behavior exercise the “no states observed” branch of ObservedOrderErr. Avoid calling terminal on an empty journey; provide the expected terminal value explicitly or use a non-empty declared journey with empty observations.test/e2e/recorder/flow.go (1)
96-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
stepsto the test file.The
stepsfunction inflow.go(lines 96–102) is called only byrecorder_internal_test.go(at lines 97, 100, 117–125). No production code in the e2e recorder module calls it. The coding guidelines state: keep code inline and add helper functions only when you test them later or re-use them elsewhere. Sincestepsis used only by tests, place it in the test file to reduce the production package surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/recorder/flow.go` around lines 96 - 102, Move the steps helper out of flow.go and into recorder_internal_test.go, preserving its current behavior and signature so the existing test call sites continue to work. Remove the production-package definition and keep the helper scoped to the tests.Source: Coding guidelines
test/e2e/go.mod (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the Kubernetes staging module versions.
Set
k8s.io/client-gotov0.36.3to matchk8s.io/apiandk8s.io/apimachinery.sigs.k8s.io/controller-runtime v0.24.1targets Kubernetesv0.36.x.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/go.mod` around lines 7 - 9, Update the k8s.io/client-go dependency in the test/e2e module to v0.36.3, matching the k8s.io/api and k8s.io/apimachinery staging versions while leaving controller-runtime unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/recorder/flow.go`:
- Around line 68-77: Guard empty journeys in Flow.Run before invoking want() or
creating the workload, returning a clear error that includes f.name and
indicates the flow declares no stops. Ensure When, WaitUntil, Do, and want
cannot reach the indexing in last() or journey access when no Reaches or At
declaration exists, while preserving existing behavior for non-empty journeys.
In `@test/e2e/recorder/recorder.go`:
- Line 206: Replace the fixed-prefix trimming in the recorder’s KartaFile
assignment with filepath.Rel against the repository root, using the existing
recorder/e2e path context to resolve the input path. Propagate or return the
filepath.Rel error from the surrounding recording flow instead of silently
keeping the original path, and preserve KartaFile as a repository-relative path.
- Around line 159-169: The retry loop around the k8sClient.Get call does not
distinguish between transient errors and NotFound, causing it to retry until
context timeout when the object has been deleted or garbage-collected. After the
Get assignment within the loop, add a check using apierrors.IsNotFound on the
gerr variable to detect NotFound errors immediately. If NotFound is detected,
set rec.failure with an appropriate message and return from the function, while
preserving the existing retry behavior with the select statement for other
transient errors.
- Around line 351-361: In the operatorVersion function, update the error
handling after the os.ReadFile call to check if the error is fs.ErrNotExist; if
the error is something else (permission denied, corruption, etc.), write a
message to the progress reporter to surface the unexpected failure before
falling back to serverVersion. Add an import for io/fs to enable the
fs.ErrNotExist check. Preserve the existing silent fallback behavior when the
file is simply absent.
- Line 71: Update the deferred cleanup around k8sClient.Delete to use a fresh
context with a finite timeout, ensuring Run cannot hang when the API server is
unresponsive. Capture any Delete error and report it through progress instead of
discarding it, while preserving cleanup after the original ctx is cancelled.
- Around line 100-109: Update the observe flow around statusSettled and rec.keep
so unsettled snapshots remain stored for replay but are excluded from rec.order
and order validation. Perform the settled-status check before recording the
observation in rec.keep, or otherwise ensure keep only appends settled CRs while
preserving checkpoint behavior for unsettled statuses.
In `@test/e2e/recorder/recording_internal_test.go`:
- Around line 58-64: In the action validation block, replace the two t.Errorf
calls on lines 60 and 63 with t.Fatalf calls. The first check validates that act
is not nil and has the expected Name and Operation.Verb properties; if it fails
and continues with t.Errorf, the subsequent dereference of act.Operation.Payload
on line 62 will panic. The second check performs an unchecked type assertion on
the payload shape; if that assertion fails it also panics. Using t.Fatalf
instead of t.Errorf ensures the test terminates immediately upon assertion
failure, preventing nil pointer dereferences and panic messages that mask the
actual test failure.
---
Nitpick comments:
In `@test/e2e/go.mod`:
- Around line 7-9: Update the k8s.io/client-go dependency in the test/e2e module
to v0.36.3, matching the k8s.io/api and k8s.io/apimachinery staging versions
while leaving controller-runtime unchanged.
In `@test/e2e/recorder/flow.go`:
- Around line 96-102: Move the steps helper out of flow.go and into
recorder_internal_test.go, preserving its current behavior and signature so the
existing test call sites continue to work. Remove the production-package
definition and keep the helper scoped to the tests.
In `@test/e2e/recorder/recorder_internal_test.go`:
- Around line 111-126: Add a table-driven test case in the existing tests table
covering an empty observed status sequence, ensuring the expected result and
error behavior exercise the “no states observed” branch of ObservedOrderErr.
Avoid calling terminal on an empty journey; provide the expected terminal value
explicitly or use a non-empty declared journey with empty observations.
In `@test/e2e/recorder/recorder.go`:
- Around line 313-318: Extend significantCR to remove controller-maintained
per-sync timestamp fields, including status.conditions[].lastTransitionTime,
lastUpdateTime, and lastHeartbeatTime, alongside the existing metadata fields.
Define the drop list based on the timestamp fields used by the actual flows
being added, while preserving the existing significantCR return behavior.
- Around line 39-54: Refactor the recorder flow to store cluster dependencies on
a Recorder instance rather than the package-level k8sClient, dynClient,
serverVersion, namespace, and progress variables. Update Bind to initialize and
return or expose a Recorder containing named dependency fields, including
distinct namespace and version fields, then update all flows and methods to read
those fields through the instance so parallel or differently configured
recorders do not share state.
- Line 286: Preserve wrapped error chains across the recorder package: in
test/e2e/recorder/recorder.go:286, change failure to error, assign errors
directly at lines 115, 134, and 175, create timeout failures with fmt.Errorf,
and have the recorder return failure at line 83 without errors.New; at
test/e2e/recorder/recorder.go:235-242, wrap k8sClient.Get with %w and include
the object name; at test/e2e/recorder/recording.go:73-94, wrap os.MkdirAll,
yaml.Marshal, and os.ReadFile errors with %w while retaining path context.
- Around line 184-187: Update the watch-event handling in watchWorkload to
filter on the event type before processing the object, explicitly skipping
watch.Bookmark events rather than relying on the *unstructured.Unstructured
assertion. Preserve processing for normal workload events and keep the existing
malformed-object guard for non-bookmark events.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: c1077c44-9115-4063-b5b0-b1050e7608c8
⛔ Files ignored due to path filters (1)
test/e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
test/e2e/go.modtest/e2e/recorder/flow.gotest/e2e/recorder/order.gotest/e2e/recorder/recorder.gotest/e2e/recorder/recorder_internal_test.gotest/e2e/recorder/recording.gotest/e2e/recorder/recording_internal_test.go
| func (f *Flow) When(gate StateCheck) *Flow { f.last().ActionPredicate = gate; return f } | ||
|
|
||
| // WaitUntil is When for the terminal stop. | ||
| func (f *Flow) WaitUntil(gate StateCheck) *Flow { f.last().ActionPredicate = gate; return f } | ||
|
|
||
| func (f *Flow) Do(action *Action) *Flow { f.last().Action = action; return f } | ||
|
|
||
| func (f *Flow) last() *journeyStep { return &f.journey[len(f.journey)-1] } | ||
|
|
||
| func (f *Flow) want() kartav1alpha1.ResourceStatus { return f.journey[len(f.journey)-1].State } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard last() and want() against an empty journey.
When, WaitUntil, and Do call last(), which indexes f.journey[len(f.journey)-1]. A flow built without a preceding Reaches or At has a nil journey, so last() panics with an index out of range. want() has the same problem, and Flow.Run calls it after k8sClient.Create succeeds. The failure then appears as an index panic instead of a declaration error.
Return a clear error or panic with a message that names the flow.
🛡️ Proposed guard
func (f *Flow) last() *journeyStep {
+ if len(f.journey) == 0 {
+ panic(fmt.Sprintf("flow %q: declare a stop with Reaches or At before When, WaitUntil, or Do", f.name))
+ }
return &f.journey[len(f.journey)-1]
}Flow.Run can reject an empty journey before it creates the workload:
if len(f.journey) == 0 {
return nil, fmt.Errorf("flow %s declares no stops", f.name)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/recorder/flow.go` around lines 68 - 77, Guard empty journeys in
Flow.Run before invoking want() or creating the workload, returning a clear
error that includes f.name and indicates the flow declares no stops. Ensure
When, WaitUntil, Do, and want cannot reach the indexing in last() or journey
access when no Reaches or At declaration exists, while preserving existing
behavior for non-empty journeys.
| Flow: f.name, | ||
| Want: string(f.want()), | ||
| Succeeded: succeeded, | ||
| KartaFile: strings.TrimPrefix(f.rec.kartaFile, "../../"), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Compute KartaFile relative to the repository root instead of trimming a fixed prefix.
strings.TrimPrefix(f.rec.kartaFile, "../../") leaves the value unchanged when the path does not start with that exact prefix. An absolute path, or a path passed from a different working directory, is then recorded as-is. KartaFile is documented as a repo-relative path in recording.go line 27, and the value is persisted into a committed fixture that the replay tests read. A wrong value breaks the replay lookup with no error at record time.
Use filepath.Rel against the repository root, and return an error when the path cannot be made relative.
🛠️ Proposed path normalization
- KartaFile: strings.TrimPrefix(f.rec.kartaFile, "../../"),
+ KartaFile: repoRelative(f.rec.kartaFile),// repoRelative renders a recorder-relative Karta path as repo-relative for the replay golden.
func repoRelative(path string) string {
root := filepath.Join(e2eRoot, "..", "..")
rel, err := filepath.Rel(root, filepath.Join(e2eRoot, path))
if err != nil {
return path
}
return rel
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/recorder/recorder.go` at line 206, Replace the fixed-prefix trimming
in the recorder’s KartaFile assignment with filepath.Rel against the repository
root, using the existing recorder/e2e path context to resolve the input path.
Propagate or return the filepath.Rel error from the surrounding recording flow
instead of silently keeping the original path, and preserve KartaFile as a
repository-relative path.
| func operatorVersion(op string) string { | ||
| b, err := os.ReadFile(filepath.Join(e2eRoot, "..", "..", "hack", "e2e", "operators", ".installed-versions")) | ||
| if err == nil { | ||
| for _, line := range strings.Split(string(b), "\n") { | ||
| if k, v, ok := strings.Cut(line, "="); ok && strings.TrimSpace(k) == op { | ||
| return strings.TrimSpace(v) | ||
| } | ||
| } | ||
| } | ||
| return serverVersion | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Report a read failure of .installed-versions.
operatorVersion ignores every os.ReadFile error and falls back to serverVersion. An absent file is the expected case for built-in workloads, and the doc comment states that. A permission error or a corrupt file produces the same silent fallback. The returned version becomes both a directory segment in RecordingPath and the Version field of the committed recording, so a wrong value writes the fixture to the wrong path.
Write a message to progress when the error is not fs.ErrNotExist.
🛠️ Proposed change
func operatorVersion(op string) string {
b, err := os.ReadFile(filepath.Join(e2eRoot, "..", "..", "hack", "e2e", "operators", ".installed-versions"))
- if err == nil {
+ if err != nil {
+ if !errors.Is(err, fs.ErrNotExist) {
+ fmt.Fprintf(progress, "read installed operator versions: %v\n", err)
+ }
+ } else {
for _, line := range strings.Split(string(b), "\n") {
if k, v, ok := strings.Cut(line, "="); ok && strings.TrimSpace(k) == op {
return strings.TrimSpace(v)
}
}
}
return serverVersion
}This adds an io/fs import.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func operatorVersion(op string) string { | |
| b, err := os.ReadFile(filepath.Join(e2eRoot, "..", "..", "hack", "e2e", "operators", ".installed-versions")) | |
| if err == nil { | |
| for _, line := range strings.Split(string(b), "\n") { | |
| if k, v, ok := strings.Cut(line, "="); ok && strings.TrimSpace(k) == op { | |
| return strings.TrimSpace(v) | |
| } | |
| } | |
| } | |
| return serverVersion | |
| } | |
| func operatorVersion(op string) string { | |
| b, err := os.ReadFile(filepath.Join(e2eRoot, "..", "..", "hack", "e2e", "operators", ".installed-versions")) | |
| if err != nil { | |
| if !errors.Is(err, fs.ErrNotExist) { | |
| fmt.Fprintf(progress, "read installed operator versions: %v\n", err) | |
| } | |
| } else { | |
| for _, line := range strings.Split(string(b), "\n") { | |
| if k, v, ok := strings.Cut(line, "="); ok && strings.TrimSpace(k) == op { | |
| return strings.TrimSpace(v) | |
| } | |
| } | |
| } | |
| return serverVersion | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/recorder/recorder.go` around lines 351 - 361, In the operatorVersion
function, update the error handling after the os.ReadFile call to check if the
error is fs.ErrNotExist; if the error is something else (permission denied,
corruption, etc.), write a message to the progress reporter to surface the
unexpected failure before falling back to serverVersion. Add an import for io/fs
to enable the fs.ErrNotExist check. Preserve the existing silent fallback
behavior when the file is simply absent.
|
|
||
| // Recorder records the flows of one workload type: build and Run a Flow per case. | ||
| type Recorder struct { | ||
| operator string |
There was a problem hiding this comment.
operator holds the operator key ("batch-job", "dynamo"), stored straight into the recording’s Operator. The version is a separate thing - operatorVersion(f.rec.operator) resolves it from .installed-versions and it lands in Version
| // Flow starts a flow seeded from a manifest (path relative to test/e2e). | ||
| func (r *Recorder) Flow(name, manifest string) *Flow { | ||
| return &Flow{rec: r, name: name, manifest: manifest} | ||
| } |
There was a problem hiding this comment.
this is NewFlow, not a method for Recorder
There was a problem hiding this comment.
the flow cant do anything without the recorder
same shape as sql.DB.Begin() handing you a *Tx
we can rename the method to NewFlow if you want the name to shout constructor , but id keep it on Recorder. wdyt ?
There was a problem hiding this comment.
I think this function outcome is a NewFlow, so that should be it.
There was a problem hiding this comment.
ok I'll change it
| } | ||
|
|
||
| // State registers a state predicate; declare states least- to most-advanced (Classify keeps the furthest match). | ||
| func (r *Recorder) State(name kartav1alpha1.ResourceStatus, match StateCheck) *Recorder { |
There was a problem hiding this comment.
Should be AddState / AppendState
There was a problem hiding this comment.
yep , you call it once per state so AddState says it better - like cobra with AddCommand. and we already have a Reader.State() getter , so a State() that adds things reads odd right next to it. will rename
| } | ||
|
|
||
| // Timeout overrides the per-flow deadline (default 3m). | ||
| func (r *Recorder) Timeout(d time.Duration) *Recorder { r.timeout = d; return r } |
There was a problem hiding this comment.
yep its a setter so SetTimeout , bare Timeout() reads like a getter (Effective Go: getters are bare , setters are SetX). will change
|
|
||
| // ObservedOrderErr checks the observed states are a legal walk of the journey: required steps appear in | ||
| // order ending at want; Optional or recurring states may be absent; anything else fails. | ||
| func ObservedOrderErr(declared []JourneyStep, observed []v1alpha1.ResourceStatus, want v1alpha1.ResourceStatus) error { |
There was a problem hiding this comment.
I understand the general concept, but I'm having trouble following. Can you break it into smaller functions
There was a problem hiding this comment.
done
pulled the “which steps are skippable” logic into skippableSteps and labeled the phases (compact -> walk -> terminal check)
| var ( | ||
| k8sClient client.Client | ||
| dynClient dynamic.Interface | ||
| serverVersion string | ||
| namespace string | ||
| progress io.Writer = io.Discard | ||
| ) |
There was a problem hiding this comment.
Why global and not part of a struct? It will be hard to follow what are the properties of the recorder
|
|
||
| // Run applies the manifest, drives the workload through the journey, and writes the recording. On a | ||
| // flow-level failure the recording is still written (succeeded:false) for triage. | ||
| func (f *Flow) Run(ctx context.Context) (*Recording, error) { |
There was a problem hiding this comment.
Shouldn't it be in flow.go?
There was a problem hiding this comment.
This is the entry point for the recording flow. You create the recorder and then call Run.
The function uses the logic declared in recorder.go, so I think we should keep it here for readability.
| if len(pending) > 0 && state == pending[0].State && | ||
| (pending[0].ActionPredicate == nil || pending[0].ActionPredicate(u)) { | ||
| if pending[0].Action != nil { | ||
| ra, err := fireAction(ctx, obj, pending[0].Action) | ||
| if err != nil { | ||
| rec.failure = err.Error() | ||
| return true | ||
| } | ||
| rec.attachAction(ra) | ||
| } | ||
| pending = pending[1:] | ||
| } |
There was a problem hiding this comment.
Wrap with a function, with a name that offers this code block logic
There was a problem hiding this comment.
done - wrapped it as fireReachedCheckpoint
|
|
||
| // A recording round-trips through WriteRecording/LoadRecording: metadata, the ordered STATE states, and a | ||
| // fired action all survive the file. | ||
| func TestRecordingRoundTrips(t *testing.T) { |
There was a problem hiding this comment.
its internal unit test for the infra , ginkgo is overkill for it
There was a problem hiding this comment.
and i want the recorder to be decoupled from any test framework so we will be able to use it in other place if needed , i think we should keep it as regular unit tests
flows test ( next pr ) will be ginko
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/recorder/flow.go`:
- Around line 29-30: Update Recorder.AddState to reject an empty name before
appending the NamedState, preserving registration only for valid state names;
add coverage confirming an empty name is rejected and cannot be classified as
UndefinedStatus.
- Around line 34-35: Update Recorder.SetTimeout to reject non-positive durations
before assigning r.timeout, preserving valid timeout configuration and
preventing already-expired flow contexts; add boundary tests covering zero,
negative, and positive durations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: a260d1e9-651e-4423-b27e-80b48e2a7d74
📒 Files selected for processing (5)
test/e2e/recorder/flow.gotest/e2e/recorder/order.gotest/e2e/recorder/recorder.gotest/e2e/recorder/recording.gotest/e2e/recorder/recording_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/recorder/recorder.go
- test/e2e/recorder/recording_internal_test.go
- test/e2e/recorder/recording.go
- test/e2e/recorder/order.go
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/recorder/recorder.go (1)
362-367: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve relative manifest paths from
test/e2e.
NewFlowdocumentsmanifestas relative totest/e2e, butreadManifestresolves it from the process working directory. Resolve the documented base directory before callingos.ReadFile, or change the contract and add a test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/recorder/recorder.go` around lines 362 - 367, Update readManifest to resolve relative manifest paths against the documented test/e2e base directory before calling os.ReadFile, while preserving absolute paths and existing error wrapping. Alternatively, change NewFlow’s manifest contract and add coverage for the new behavior.
🧹 Nitpick comments (1)
test/e2e/recorder/recorder_internal_test.go (1)
18-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific validation contract.
Both tests pass for any panic. A panic caused by an unrelated defect would therefore satisfy the tests. Assert the expected panic message, and add a positive
SetTimeoutcase to confirm valid durations remain accepted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/recorder/recorder_internal_test.go` around lines 18 - 38, Update TestAddStateRejectsEmptyName and TestSetTimeoutRejectsNonPositive to assert the exact expected panic messages rather than accepting any panic, while preserving their invalid-input coverage. Extend the SetTimeout tests with a positive duration case that must complete without panicking, confirming valid timeouts remain accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/recorder/flow.go`:
- Around line 48-53: Update Recorder.AddState to reject a nil StateCheck before
appending the state, using the same validation approach as the empty name check.
Ensure Classify never receives a NamedState with a nil Match; add or update
coverage for the nil-match validation.
- Around line 39-46: Update New to validate the version argument before
constructing the Recorder, rejecting an empty version (or applying an explicit,
stable default) so RecordingPath always contains a unique version segment.
Preserve the existing cluster progress initialization and Recorder field
assignments for valid versions.
In `@test/e2e/recorder/recorder.go`:
- Around line 196-200: Update the New function to validate that
f.rec.cluster.OutputDir is non-empty, alongside the existing Progress
validation, and return an appropriate error when it is missing. Keep recording
creation unchanged once both required cluster configuration values are present.
---
Outside diff comments:
In `@test/e2e/recorder/recorder.go`:
- Around line 362-367: Update readManifest to resolve relative manifest paths
against the documented test/e2e base directory before calling os.ReadFile, while
preserving absolute paths and existing error wrapping. Alternatively, change
NewFlow’s manifest contract and add coverage for the new behavior.
---
Nitpick comments:
In `@test/e2e/recorder/recorder_internal_test.go`:
- Around line 18-38: Update TestAddStateRejectsEmptyName and
TestSetTimeoutRejectsNonPositive to assert the exact expected panic messages
rather than accepting any panic, while preserving their invalid-input coverage.
Extend the SetTimeout tests with a positive duration case that must complete
without panicking, confirming valid timeouts remain accepted.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: bd224032-8ff6-47f2-aff6-3f75ba7f9f27
📒 Files selected for processing (3)
test/e2e/recorder/flow.gotest/e2e/recorder/recorder.gotest/e2e/recorder/recorder_internal_test.go
| defer func() { | ||
| delCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second) | ||
| defer cancel() | ||
| if err := f.rec.cluster.Client.Delete(delCtx, obj); err != nil && !apierrors.IsNotFound(err) { | ||
| fmt.Fprintf(f.rec.log, "cleanup: delete %s/%s failed: %v\n", obj.GetNamespace(), obj.GetName(), err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Create a function to handle deletion and defer that
| } | ||
| rec.keep(u, state) | ||
| var stop bool | ||
| if pending, stop = f.fireReachedCheckpoint(ctx, obj, u, state, pending, rec); stop { |
There was a problem hiding this comment.
code now is much more readable
|
|
||
| // Recorder records the flows of one workload type: build and Run a Flow per case. | ||
| type Recorder struct { | ||
| operator string |
|
|
||
| // Recorder records the flows of one workload type: build and Run a Flow per case. | ||
| type Recorder struct { | ||
| cluster Cluster |
There was a problem hiding this comment.
isn't cluster+outputDir +log is config ?
There was a problem hiding this comment.
yep - recorder holds the config now
|
|
||
| // New starts a recorder from cfg; version is stamped on the recording and kartaFile is recorded as metadata | ||
| // for the replay golden (neither path is read here). | ||
| func New(cfg Config, operator, version, kartaName, kartaFile string) *Recorder { |
There was a problem hiding this comment.
does it make sense that ctor of recorder is in flow.go ?
There was a problem hiding this comment.
moved new into recorder.go in the restructure
| version: version, | ||
| kartaName: kartaName, | ||
| kartaFile: kartaFile, | ||
| timeout: 3 * time.Minute, |
There was a problem hiding this comment.
let's not make it inline - I think it worth a config (arg?)
| journey []journeyStep | ||
| } | ||
|
|
||
| func (f *Flow) Reaches(state kartav1alpha1.ResourceStatus) *Flow { |
There was a problem hiding this comment.
This is a builder pattern, maybe the flow should only contain the attributes and have a FlowBuilder. With this exact design pattern, this is the practice.
There was a problem hiding this comment.
yeah thats the classic java/GoF builder - a separate builder + a product struct. in go the common practice is a single fluent type with a terminal , not a separate FlowBuilder. controller-runtime (what we build on) does exactly this - ControllerManagedBy(mgr).For(...).Owns(...).Complete(r) , one Builder , every method returns it , Complete() is the terminal. Flow is the same shape (NewFlow -> Reaches/At -> Run) , so id keep it as is. wdyt ?
For() returns the same builder , fluent-on-self , no separate product
https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis
go idiom for construction is functional options , not GoF builders
| } | ||
|
|
||
| // openWatch starts a resilient watch of the workload by name that resumes after transient drops. | ||
| func (f *Flow) openWatch(ctx context.Context, workload *unstructured.Unstructured) (watch.Interface, error) { |
There was a problem hiding this comment.
openWatch actually starts watching, so a better name is startWatch, sound like a sport thingy
| // keep dedups on real changes. | ||
| func significantFields(cr *unstructured.Unstructured) map[string]any { | ||
| stripped := cr.DeepCopy().Object | ||
| unstructured.RemoveNestedField(stripped, "metadata", "resourceVersion") |
There was a problem hiding this comment.
nit- make top level constant list of the fields ?
There was a problem hiding this comment.
done - pulled them into a top level volatileFields var
| // isStatusSettled reports whether the controller has caught up (observedGeneration >= generation); workloads | ||
| // without those fields count as settled. | ||
| func isStatusSettled(cr *unstructured.Unstructured) bool { | ||
| gen, hasGen, _ := unstructured.NestedInt64(cr.Object, "metadata", "generation") |
There was a problem hiding this comment.
you could use cr.GetGeneration
There was a problem hiding this comment.
done for the generation side. observedGeneration has no getter (status is type specific) so NestedInt64 stays for that half
|
|
||
| // isStatusSettled reports whether the controller has caught up (observedGeneration >= generation); workloads | ||
| // without those fields count as settled. | ||
| func isStatusSettled(cr *unstructured.Unstructured) bool { |
There was a problem hiding this comment.
renamed it isWorkloadObserved - it checks the controller observed the current spec (observedGeneration >= generation). and per the record thread , it now gates only the judgment , not the recording
| // action if the workload just reached it, and reports whether the flow is finished (or an action failed). | ||
| func (o *observation) record(ctx context.Context, cr *unstructured.Unstructured) (done bool) { | ||
| o.lastSeen = cr | ||
| if !isStatusSettled(cr) { |
There was a problem hiding this comment.
I think it could be an issue with workload with high rate of write - and you don't record during the whole time - what's wrong with saving the workload when status is not synced yet ?
There was a problem hiding this comment.
youre right , changed it. every distinct frame is recorded now (marked unobserved in the yaml) , and only the judgment - order check , checkpoint actions , terminal - waits for observedGeneration >= generation. right after we patch the spec the status still describes the old spec , so judging that frame is timing luck , recording it is fine
verified on a live cluster - the deployment scale flow now captures the mid transition frames and the replay passes on them too. btw generation only bumps on spec changes , so a workload writing status at a high rate never gets starved by the gate
| return observed >= gen | ||
| } | ||
|
|
||
| // blankWithGVK returns a fresh object carrying only src's GVK, so a merge-patch or a Get never sends back a |
There was a problem hiding this comment.
| // blankWithGVK returns a fresh object carrying only src's GVK, so a merge-patch or a Get never sends back a | |
| // blankWithGVK returns a fresh object carrying only src's GVK |
| return name | ||
| } | ||
|
|
||
| // journeyStep is one stop on a journey. ActionPredicate lets the same state appear more than once (a scale |
There was a problem hiding this comment.
place it on top where you use it as part of flow
There was a problem hiding this comment.
done - journeyStep and the Action consts sit next to Flow now
| // journeyStep is one stop on a journey. ActionPredicate lets the same state appear more than once (a scale | ||
| // flow is Running at 1, 3, then 1): the step is reached only once the predicate holds, firing its action. | ||
| type journeyStep struct { | ||
| State kartav1alpha1.ResourceStatus |
There was a problem hiding this comment.
I think it's not clear what is state as it Karta.resourceStatus
There was a problem hiding this comment.
the enum of karta
Running / Degraded etc..
| } | ||
|
|
||
| // When gates the current stop on a predicate over the workload's own fields. | ||
| func (f *Flow) When(gate StateCheck) *Flow { f.last().ActionPredicate = gate; return f } |
There was a problem hiding this comment.
maybe a better api would be
step(state, action? )
There was a problem hiding this comment.
Step(Running, Gate(ReplicasReady(1)), Action(ScaleReplicas(3)))vs today:
At(Running).When(ReplicasReady(1)).Do(ScaleReplicas(3))| switch { | ||
| case !open: | ||
| var stop bool | ||
| if watcher, stop = o.reconnect(ctx, watcher, lastWatchErr); stop { |
There was a problem hiding this comment.
it's weird to me that you need to deal with it - isn't there a solution from controller runtime to reconnect ?
There was a problem hiding this comment.
controller-runtime reconnects via informers , and informers are level driven - on reconnect they relist and hand you the latest state , collapsing whats in between. fine for a reconciler , fatal for a recorder whose whole job is the intermediate states. RetryWatcher is the edge tool (every event after rv X) and it self reconnects on transient drops - our reconnect only covers the one case it cant , see the reconnect thread
https://github.com/kubernetes/client-go/blob/v0.36.2/tools/watch/retrywatcher.go#L113-L116
- RetryWatcher restarts from lastResourceVersion , no gap on transient drops
| return state == o.flow.want() && len(o.pending) == 0 | ||
| } | ||
|
|
||
| // reconnect handles a dropped watch: it re-fetches the workload for a fresh resourceVersion, records that |
There was a problem hiding this comment.
I think that you don't want to get the fresh ResourceVersion but continue from last observation
There was a problem hiding this comment.
continuing from the last observed rv is what RetryWatcher already does on transient drops. this path runs only after it gave up , and the only give up on a live connection is 410 gone - the server refused that rv (compacted). retrying it loops on 410 forever , the recovery is fresh get + watch from the new rv. if a state fell in the gap the order check fails the flow loudly. added a comment
https://github.com/kubernetes/client-go/blob/v0.36.2/tools/watch/retrywatcher.go#L245-L249
- "Never retry RV too old errors" , 410 ends the RetryWatcher
https://kubernetes.io/docs/reference/using-api/api-concepts/#410-gone-responses
- the prescribed 410 recovery: fresh get , then watch from the returned rv
The recorder drives a workload through a declared flow and records the CRs it passes through as a STATE/ACTION event stream, judging each state from the workload's own fields (never from Karta) so it stays decoupled from the library it feeds. Ginkgo/Gomega-free: failures return errors, progress goes to an injected writer. Standalone module; flows and replay land in follow-up PRs. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
CodeRabbit: - bound the cleanup Delete with a timeout and log its failure - record only settled CRs: check statusSettled before keep - reject an empty flow in Run instead of panicking in want()/last() - fail fast when the re-list returns NotFound instead of retrying to the deadline - wrap the remaining bare errors with %w (watch Get, WriteRecording, LoadRecording) - make the round-trip test action check t.Fatalf so a nil action doesnt panic rogirun: - State -> AddState (cumulative, like cobra AddCommand) - Timeout -> SetTimeout (setter; bare Timeout reads like a getter per Effective Go) - Flow method -> NewFlow free func (standard Go constructor shape, like bufio.NewReader) - extract skippableSteps from ObservedOrderErr + label the walk phases - extract fireReachedCheckpoint from the observe loop Signed-off-by: aviadh <aviad.hayumi@gmail.com>
…obals Replace the package-level k8sClient/dynClient/serverVersion/namespace/progress vars and Bind() with a Cluster struct passed to New(). The suite builds one Cluster in BeforeSuite and passes it in, so the recorder library no longer owns ambient global state; watch/fireAction/operatorVersion read it through the recorder. Matches how stdlib constructors take their deps (bufio.NewReader) and removes the version/ns transposition risk in Bind's positional args. No behavior change. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
…uilder
AddState("") would collide with Classify's no-match sentinel and be recorded as
Undefined; SetTimeout(<=0) would expire the flow context before it observes
anything. Both are chainable setters, so they panic on the bad input (like
regexp.MustCompile), with boundary tests. Addresses CodeRabbit.
Signed-off-by: aviadh <aviad.hayumi@gmail.com>
Drop const e2eRoot. The recorder no longer reads hack/e2e/.installed-versions, hardcodes recorded_data, or joins manifest paths to a magic root. Instead: version is passed to New, the output dir is Cluster.OutputDir, and readManifest reads the path as given. Take-only-the-recorder now has no test/e2e coupling. No behavior change. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
… Config Cluster is now just Client/Dynamic/Namespace. OutputDir and the progress writer (renamed Log, still io.Writer since it only carries progress and warning lines) move to a new Config that wraps Cluster and is what New takes - neither belongs in cluster access. Pure rename, no behavior change. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
make check only ran the main module's tests; the test/e2e recorder tests went uncovered in CI. Add a test-e2e target that builds and tests the e2e module (excluding the cluster-driven flows, which are compile-checked) and wire it into check so the existing CI make check step runs it. No workflow change. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
New now panics on an empty operator/version/kartaName/kartaFile (they form the recording path and metadata) or an empty Config.OutputDir (it would write to a cwd path). AddState panics on a nil predicate (Classify would nil-panic on it). When and WaitUntil still accept nil, where it means match on state alone. Boundary tests added. Addresses CodeRabbit. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
… clearer names observe's closures and the overloaded rec/f.rec become an observation struct (observation.go) whose methods drive and record one watched run: follow, record, fireCheckpoint, reconnect, relist. fireReachedCheckpoint's six params collapse to fireCheckpoint(ctx, state, cr). Renames: obj->workload, u->cr, significantCR-> significantFields, capture->snapshot, GVKOnly->gvkOnly, seed->current; readManifest and the inline delete become applyManifest and deleteWorkload; deep field chains use f.client()/f.log(). Functions ordered most-important-first per file. No behavior change; recorder unit tests and the replay golden stay green. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
Rename identifiers to say what they do, collapse the duplicated order check, and give each file one job. No behavior change. - createWorkload (was applyManifest), blankWithGVK (was gvkOnly), isStatusSettled, hasReachedTerminal, refetch (was relist, a Get not a LIST), attachAction. - Unexport internals with no outside caller: classify, namedState, and the recording plumbing (schemaVersion, recordingPath, writeRecording, loadRecording, newReader, Recording.states). - Collapse the order check: drop JourneyStep and the journeySteps adapter; observedOrderErr takes []journeyStep directly and is unexported. - New doc.go (package comment) and cr.go (the unstructured-CR helpers); move openWatch/fireAction beside their callers in observation.go; move the recorder setup into recorder.go so flow.go is only the authoring DSL. The recorder unit tests stay green. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
performAction (was fireAction) and advanceCheckpoint (was fireCheckpoint), with the comments reworded to match. Both are unexported and used only inside observation.go; no behavior change. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
Address Ron Lev's review on the recorder: - Recorder keeps the whole Config (cluster, outputDir, log are the config) rather than re-flattening its three fields; accessors read f.rec.config.X. - Add Config.Timeout for the per-flow deadline (defaults to the defaultTimeout const when unset); SetTimeout still overrides it per recorder. - Comment the operator/version/kartaName/kartaFile fields so operator (the key, e.g. "batch-job") reads distinctly from the separately-resolved version. The "constructor of Recorder in flow.go" point is already handled: New lives in recorder.go after the readability restructure. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
operator/version/kartaName/kartaFile passed to New were only used to write the
recording; the recorder never read them while driving. Take them out:
- New(cfg) takes just the Config.
- Fixture{Operator, Version, KartaName, KartaFile} carries the catalog labeling.
- Run returns the observed *Recording without writing it.
- Recorder.Save(fx, rec) stamps the fixture and writes it under the fixtures tree,
passed or failed, so a failed flow still leaves its triage artifact (a nil
recording is a no-op).
Recorded YAML is unchanged (pure API reshape).
Signed-off-by: aviadh <aviad.hayumi@gmail.com>
It starts the watch; startWatch says what it does. Unexported, used only in observation.go. Review nit from Roee (rogirun). Signed-off-by: aviadh <aviad.hayumi@gmail.com>
45fb10e to
4fa074a
Compare
4fa074a to
c2622eb
Compare
Ron Lev's review round on the recorder: - record frames whose controller has not observed the spec yet (observedGeneration < generation), marked staleObservedGeneration in the recording; the judgment - order check, checkpoint actions, terminal - still waits for an observed frame. Verified on a live cluster: a deployment scale flow captures the mid-transition frames and the replay asserts them. - rename isStatusSettled to isWorkloadObserved and Maybe to OptionalReaches; merge WaitUntil into When (identical bodies); the flow suites follow. - volatileFields var for the dedup drop-list; GetGeneration for the spec side; journeyStep and the Action vocabulary move next to Flow. - document Flow, Reaches, the ResourceStatus vocabulary borrow, the dumpStatus indentation; add a README with the files and the flow of a run. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
c2622eb to
866ac3e
Compare
3 participants
first piece of the e2e recorder - just the
recorderpackage , a new go module undertest/e2e, no flows or recorded data yet.it runs a workload on a cluster , watches it , and writes every distinct CR it saw as a STATE/ACTION stream.
the state is read from the workloads own fields , never from Karta , so later we replay it through Karta and check it reads the same.
no Karta and no Ginkgo/Gomega in here , errors come back plain.
part of #137 , first piece of #139.
review in this order :
flow.go- the api you write a test with : the journey chain (Reaches/Maybe/At/When/Do) , states and actionsrecorder.go- the setup + engine :Newtakes the cluster config ,Rundrives a flow end to end ,Savewrites the recordingobservation.go- one live run : watches the workload , keeps every distinct settled CR , performs checkpoint actions , survives watch dropscr.go- small helpers over an unstructured CR (significant fields , settled status , blank object with gvk)order.go- one check : the states came in the order we declaredrecording.go- the on disk format + the reader that walks it back for replaydoc.go- just the package comment*_test.go- offline unit tests for all of it , no cluster neededSummary by CodeRabbit
New Features
Tests