Repository navigation
OSAC-1459: Extend provisioning lifecycle for multi-job tracking - #147
Merged
Merged
Conversation
Assisted-by: Cursor/Claude
|
@SiddarthR56: This pull request references OSAC-1459 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
WalkthroughThe change adds an optional target to job status records and CRD schemas. Provisioning and deprovisioning lifecycles now track jobs, retries, polling, validation, errors, and requeues independently for multiple targets. ChangesMulti-target provisioning lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant ProvisioningLifecycle
participant TargetProvider
participant ResourceStatus
Controller->>ProvisioningLifecycle: run target list
ProvisioningLifecycle->>TargetProvider: evaluate and process each target
TargetProvider-->>ProvisioningLifecycle: job result and requeue
ProvisioningLifecycle->>ResourceStatus: flush status after job creation
ProvisioningLifecycle-->>Controller: earliest requeue and aggregated error
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
osac-operator/pkg/provisioning/provision_lifecycle.go (2)
96-108: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the API server read failure instead of discarding it.
If
apiReader.Getfails, the function returnsfalse. The caller then treats the resource as having no in-flight job and triggers a new provision job. That is exactly the duplicate-job case this guard exists to prevent. Record the error so operators can see why the guard was bypassed.♻️ Proposed fix
log := ctrllog.FromContext(ctx) if err := apiReader.Get(ctx, key, fresh); err != nil { + log.Error(err, "failed to read resource from API server; proceeding without duplicate-job check", "target", target) return false }As per path instructions "Never ignore error returns".
🤖 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 `@osac-operator/pkg/provisioning/provision_lifecycle.go` around lines 96 - 108, Update CheckAPIServerForNonTerminalProvisionJobAndTarget to log the error returned by apiReader.Get before returning false, including the resource key and error details so operators can identify why the guard was bypassed. Preserve the existing return behavior.Source: Path instructions
229-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPut
errorlast in therunLifecycleCoresignature.The function returns
(ctrl.Result, error, bool). Go convention placeserroras the final return value, and revive'serror-returnrule flags this shape. The function is unexported and has two call sites, so the change is contained.♻️ Proposed fix
-) (ctrl.Result, error, bool) { +) (ctrl.Result, bool, error) { action, latestJob := evaluateActionForTarget(provState, target, checkAPIServer) @@ switch action { case Skip: - return ctrl.Result{}, nil, false + return ctrl.Result{}, false, nil case Trigger: res, err := trigger() - return res, err, triggered + return res, triggered, err case Requeue: - return ctrl.Result{RequeueAfter: pollInterval}, nil, false + return ctrl.Result{RequeueAfter: pollInterval}, false, nil case Backoff: res, err := handleBackoffForTarget(ctx, provState, target, latestJob, trigger) - return res, err, triggered + return res, triggered, err default: // Poll res, err := PollJob(ctx, provider, resource, provState, latestJob, pollInterval, callbacks) - return res, err, false + return res, false, nil }Update both call sites accordingly:
- result, err, triggered := runLifecycleCore(ctx, provider, resource, provState, "", maxHistory, pollInterval, callbacks, checkAPIServer) + result, triggered, err := runLifecycleCore(ctx, provider, resource, provState, "", maxHistory, pollInterval, callbacks, checkAPIServer)- res, err, triggered := runLifecycleCore(ctx, t.Provider, resource, provState, t.Name, maxHistory, pollInterval, t.Callbacks, t.CheckAPIServer) + res, triggered, err := runLifecycleCore(ctx, t.Provider, resource, provState, t.Name, maxHistory, pollInterval, t.Callbacks, t.CheckAPIServer)As per path instructions for Go files.
🤖 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 `@osac-operator/pkg/provisioning/provision_lifecycle.go` around lines 229 - 275, Change runLifecycleCore to return (ctrl.Result, bool, error), placing error last, and update both call sites to unpack and return the values in the new order. Preserve the existing result, triggered, and error semantics across every action branch.Source: Path instructions
osac-operator/pkg/provisioning/provision_lifecycle_test.go (2)
1201-1226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe mock discards
provisionJobs, so cross-target leakage cannot be asserted.
mockProvider.TriggerDeprovisiondrops itsprovisionJobsargument before callingtriggerDeprovisionFunc. No test in this suite can therefore observe what the lifecycle passes to a provider. That is why the unscoped*jobsargument intriggerDeprovisionJobForTargethas no failing test.Widen the mock's callback to accept
provisionJobs, then assert that a target's provider only receives its own provision jobs.♻️ Proposed fix
- triggerDeprovisionFunc func(ctx context.Context, resource client.Object) (*DeprovisionResult, error) + triggerDeprovisionFunc func(ctx context.Context, resource client.Object, provisionJobs []v1alpha1.JobStatus) (*DeprovisionResult, error)func (m *mockProvider) TriggerDeprovision(ctx context.Context, resource client.Object, provisionJobs []v1alpha1.JobStatus) (*DeprovisionResult, error) { - return m.triggerDeprovisionFunc(ctx, resource) + return m.triggerDeprovisionFunc(ctx, resource, provisionJobs) }Update every
triggerDeprovisionFuncliteral to the new signature, then add:ginkgo.It("passes only the target's own provision jobs to that target's provider", func() { var fabricSaw []v1alpha1.JobStatus fabricProvider := &mockProvider{ triggerDeprovisionFunc: func(_ context.Context, _ client.Object, pj []v1alpha1.JobStatus) (*DeprovisionResult, error) { fabricSaw = pj return &DeprovisionResult{Action: DeprovisionTriggered, JobID: "fabric-deprov"}, nil }, } jobs := []v1alpha1.JobStatus{ {JobID: "fabric-prov", Type: v1alpha1.JobTypeProvision, Target: "fabric", State: v1alpha1.JobStateRunning, Timestamp: metav1.NewTime(time.Now())}, {JobID: "k8s-prov", Type: v1alpha1.JobTypeProvision, Target: "k8s", State: v1alpha1.JobStateRunning, Timestamp: metav1.NewTime(time.Now())}, } targets := []DeprovisionTarget{{Name: "fabric", Provider: fabricProvider}} _, _, err := RunMultiTargetDeprovisioningLifecycle(ctx, targets, resource, &jobs, maxHistory, pollInterval) Expect(err).NotTo(HaveOccurred()) for _, j := range fabricSaw { Expect(j.Target).To(Equal("fabric")) } })🤖 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 `@osac-operator/pkg/provisioning/provision_lifecycle_test.go` around lines 1201 - 1226, Update mockProvider’s triggerDeprovisionFunc callback signature and every corresponding literal to receive provisionJobs, forwarding that argument from TriggerDeprovision. Add a test around RunMultiTargetDeprovisioningLifecycle that supplies provision jobs for multiple targets and verifies the provider for one target receives only jobs whose Target matches that target.
281-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for pre-existing untagged job history.
This suite always starts from either an empty job list or jobs already tagged with
Target. No test covers a resource that carries a legacy job withTarget: ""while the multi-target lifecycle runs. That is the upgrade path for every resource provisioned before this change.Once you decide the migration behavior for untagged history, encode it here. The test makes the chosen contract explicit and prevents a silent re-provision regression.
ginkgo.It("<states the decided behavior for a legacy untagged succeeded provision job>", func() { resource := &v1alpha1.Subnet{} jobs := []v1alpha1.JobStatus{ {JobID: "legacy-1", Type: v1alpha1.JobTypeProvision, Target: "", State: v1alpha1.JobStateSucceeded, ConfigVersion: "v1", Timestamp: metav1.NewTime(time.Now())}, } // ... assert whether "fabric" re-triggers or reuses the untagged job. })Do you want me to write this test once the migration behavior is settled?
🤖 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 `@osac-operator/pkg/provisioning/provision_lifecycle_test.go` around lines 281 - 306, Add a Ginkgo test alongside the existing RunMultiTargetProvisioningLifecycle tests that seeds provState.Jobs with a succeeded legacy provision JobStatus whose Target is empty, then runs the multi-target lifecycle and asserts the explicitly chosen migration behavior for both targets—whether the untagged history is reused or each target is re-triggered. Include the existing config version and timestamp fields so the test represents a real upgrade path.
🤖 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 `@osac-operator/pkg/provisioning/provision_lifecycle.go`:
- Around line 426-438: Filter jobs by matching Target before passing them from
triggerDeprovisionJobForTarget to TriggerDeprovision. In
osac-operator/pkg/provisioning/provision_lifecycle.go lines 426-438, pass only
the target’s provision jobs; in
osac-operator/pkg/provisioning/provision_lifecycle_test.go lines 1201-1226,
update mockProvider.triggerDeprovisionFunc to accept and forward provisionJobs,
then assert each provider receives only its own jobs.
- Around line 304-347: Choose and implement one migration strategy for legacy
jobs with Target == ""—backfill targets or treat untagged history as belonging
to the designated transition target—so existing resources are not reprovisioned
and deletions are not blocked. Document the chosen behavior in
RunMultiTargetProvisioningLifecycle at
osac-operator/pkg/provisioning/provision_lifecycle.go:304-347 and apply it in
RunMultiTargetDeprovisioningLifecycle at
osac-operator/pkg/provisioning/provision_lifecycle.go:593-639. Add coverage in
osac-operator/pkg/provisioning/provision_lifecycle_test.go:281-306 using
untagged jobs for both provisioning and deprovisioning, asserting the selected
behavior.
---
Nitpick comments:
In `@osac-operator/pkg/provisioning/provision_lifecycle_test.go`:
- Around line 1201-1226: Update mockProvider’s triggerDeprovisionFunc callback
signature and every corresponding literal to receive provisionJobs, forwarding
that argument from TriggerDeprovision. Add a test around
RunMultiTargetDeprovisioningLifecycle that supplies provision jobs for multiple
targets and verifies the provider for one target receives only jobs whose Target
matches that target.
- Around line 281-306: Add a Ginkgo test alongside the existing
RunMultiTargetProvisioningLifecycle tests that seeds provState.Jobs with a
succeeded legacy provision JobStatus whose Target is empty, then runs the
multi-target lifecycle and asserts the explicitly chosen migration behavior for
both targets—whether the untagged history is reused or each target is
re-triggered. Include the existing config version and timestamp fields so the
test represents a real upgrade path.
In `@osac-operator/pkg/provisioning/provision_lifecycle.go`:
- Around line 96-108: Update CheckAPIServerForNonTerminalProvisionJobAndTarget
to log the error returned by apiReader.Get before returning false, including the
resource key and error details so operators can identify why the guard was
bypassed. Preserve the existing return behavior.
- Around line 229-275: Change runLifecycleCore to return (ctrl.Result, bool,
error), placing error last, and update both call sites to unpack and return the
values in the new order. Preserve the existing result, triggered, and error
semantics across every action branch.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 02cb27f9-7baa-4299-862e-918b3ca93a2e
📒 Files selected for processing (25)
osac-operator/api/v1alpha1/job_types.goosac-operator/charts/operator-crds/templates/osac.openshift.io_clusterorders.yamlosac-operator/charts/operator-crds/templates/osac.openshift.io_computeinstances.yamlosac-operator/charts/operator-crds/templates/osac.openshift.io_externalipattachments.yamlosac-operator/charts/operator-crds/templates/osac.openshift.io_externalippools.yamlosac-operator/charts/operator-crds/templates/osac.openshift.io_externalips.yamlosac-operator/charts/operator-crds/templates/osac.openshift.io_natgateways.yamlosac-operator/charts/operator-crds/templates/osac.openshift.io_securitygroups.yamlosac-operator/charts/operator-crds/templates/osac.openshift.io_subnets.yamlosac-operator/charts/operator-crds/templates/osac.openshift.io_tenants.yamlosac-operator/charts/operator-crds/templates/osac.openshift.io_virtualnetworks.yamlosac-operator/config/crd/bases/osac.openshift.io_clusterorders.yamlosac-operator/config/crd/bases/osac.openshift.io_computeinstances.yamlosac-operator/config/crd/bases/osac.openshift.io_externalipattachments.yamlosac-operator/config/crd/bases/osac.openshift.io_externalippools.yamlosac-operator/config/crd/bases/osac.openshift.io_externalips.yamlosac-operator/config/crd/bases/osac.openshift.io_natgateways.yamlosac-operator/config/crd/bases/osac.openshift.io_securitygroups.yamlosac-operator/config/crd/bases/osac.openshift.io_subnets.yamlosac-operator/config/crd/bases/osac.openshift.io_tenants.yamlosac-operator/config/crd/bases/osac.openshift.io_virtualnetworks.yamlosac-operator/pkg/provisioning/job_helpers.goosac-operator/pkg/provisioning/provision_helpers.goosac-operator/pkg/provisioning/provision_lifecycle.goosac-operator/pkg/provisioning/provision_lifecycle_test.go
Comment on lines
+304
to
+347
| // RunMultiTargetProvisioningLifecycle runs the same evaluate/trigger/poll/ | ||
| // backoff state machine as RunProvisioningLifecycle independently for each | ||
| // target, against job history tagged by JobStatus.Target. Requeues at the | ||
| // soonest interval any target still needs (ctrl.Result{} only once every | ||
| // target has reached Skip), and joins errors from all targets rather than | ||
| // short-circuiting on the first one — one target's failure never prevents | ||
| // another target's attempt in the same call. statusFlush is called at most | ||
| // once per call, only if at least one target triggered a new job. | ||
| // | ||
| // Every target's manager must have actually been dispatched to before being | ||
| // included here — a target that was never dispatched (e.g. a resource with | ||
| // no k8s manager configured) has no job history to evaluate and should be | ||
| // omitted by the caller rather than passed in. | ||
| func RunMultiTargetProvisioningLifecycle( | ||
| ctx context.Context, | ||
| targets []JobTarget, | ||
| resource client.Object, | ||
| provState *State, | ||
| maxHistory int, | ||
| pollInterval time.Duration, | ||
| statusFlush func() error, | ||
| ) (ctrl.Result, error) { | ||
| if err := validateJobTargets(targets); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| var ( | ||
| errs []error | ||
| anyTriggered bool | ||
| result ctrl.Result | ||
| ) | ||
|
|
||
| for _, t := range targets { | ||
| res, err, triggered := runLifecycleCore(ctx, t.Provider, resource, provState, t.Name, maxHistory, pollInterval, t.Callbacks, t.CheckAPIServer) | ||
| if err != nil { | ||
| errs = append(errs, fmt.Errorf("target %q: %w", t.Name, err)) | ||
| } | ||
| if triggered { | ||
| anyTriggered = true | ||
| } | ||
| if res.RequeueAfter > 0 && (result.RequeueAfter == 0 || res.RequeueAfter < result.RequeueAfter) { | ||
| result.RequeueAfter = res.RequeueAfter | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Pre-change job history is unreachable from every multi-target path. FindLatestJobByTypeAndTarget matches Target on exact string equality, and both validators reject an empty Name. Every job persisted before this change carries Target == "", so no multi-target lookup can ever find it. One migration decision — backfill Target on existing records, or treat untagged history as belonging to a designated target during a transition window — resolves all three sites.
osac-operator/pkg/provisioning/provision_lifecycle.go#L304-L347: document the chosen migration behavior in theRunMultiTargetProvisioningLifecycledoc comment, next to the existing "never dispatched" note, and implement it so an already-provisioned resource is not re-provisioned on upgrade.osac-operator/pkg/provisioning/provision_lifecycle.go#L593-L639: apply the same behavior inRunMultiTargetDeprovisioningLifecycle. Here an unmatched legacy deprovision job triggers a second teardown,donestaysfalse, and the finalizer is never removed, so the CR blocks deletion.osac-operator/pkg/provisioning/provision_lifecycle_test.go#L281-L306: add a test that starts from a job withTarget: ""and asserts the decided behavior, for both the provisioning and the deprovisioning suite.
📍 Affects 2 files
osac-operator/pkg/provisioning/provision_lifecycle.go#L304-L347(this comment)osac-operator/pkg/provisioning/provision_lifecycle.go#L593-L639osac-operator/pkg/provisioning/provision_lifecycle_test.go#L281-L306
🤖 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 `@osac-operator/pkg/provisioning/provision_lifecycle.go` around lines 304 -
347, Choose and implement one migration strategy for legacy jobs with Target ==
""—backfill targets or treat untagged history as belonging to the designated
transition target—so existing resources are not reprovisioned and deletions are
not blocked. Document the chosen behavior in RunMultiTargetProvisioningLifecycle
at osac-operator/pkg/provisioning/provision_lifecycle.go:304-347 and apply it in
RunMultiTargetDeprovisioningLifecycle at
osac-operator/pkg/provisioning/provision_lifecycle.go:593-639. Add coverage in
osac-operator/pkg/provisioning/provision_lifecycle_test.go:281-306 using
untagged jobs for both provisioning and deprovisioning, asserting the selected
behavior.
Comment on lines
+426
to
438
| func triggerDeprovisionJobForTarget(ctx context.Context, provider ProvisioningProvider, resource client.Object, | ||
| jobs *[]v1alpha1.JobStatus, target string, maxHistory int, pollInterval time.Duration) (ctrl.Result, error) { | ||
| log := ctrllog.FromContext(ctx) | ||
| log.Info("triggering deprovision job") | ||
| log.Info("triggering deprovision job", "target", target) | ||
|
|
||
| result, err := provider.TriggerDeprovision(ctx, resource, *jobs) | ||
| if err != nil { | ||
| if rateLimitErr, ok := AsRateLimitError(err); ok { | ||
| log.Info("deprovision request rate-limited, requeueing", "retryAfter", rateLimitErr.RetryAfter) | ||
| log.Info("deprovision request rate-limited, requeueing", "target", target, "retryAfter", rateLimitErr.RetryAfter) | ||
| return ctrl.Result{RequeueAfter: rateLimitErr.RetryAfter}, nil | ||
| } | ||
| return ctrl.Result{}, fmt.Errorf("failed to trigger deprovision: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
TriggerDeprovision still receives every target's provision jobs. triggerDeprovisionJobForTarget passes *jobs unfiltered, while the ProvisioningProvider contract says the provider uses that slice to find and cancel a running provision job. One target's provider can therefore cancel another target's job. The test mock hides this because it drops the argument.
osac-operator/pkg/provisioning/provision_lifecycle.go#L426-L438: filter the slice to provision jobs whoseTargetequalstargetbefore passing it toprovider.TriggerDeprovision.osac-operator/pkg/provisioning/provision_lifecycle_test.go#L1201-L1226: widenmockProvider.triggerDeprovisionFuncto acceptprovisionJobs, forward the real argument, and assert that each target's provider receives only its own provision jobs.
📍 Affects 2 files
osac-operator/pkg/provisioning/provision_lifecycle.go#L426-L438(this comment)osac-operator/pkg/provisioning/provision_lifecycle_test.go#L1201-L1226
🤖 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 `@osac-operator/pkg/provisioning/provision_lifecycle.go` around lines 426 -
438, Filter jobs by matching Target before passing them from
triggerDeprovisionJobForTarget to TriggerDeprovision. In
osac-operator/pkg/provisioning/provision_lifecycle.go lines 426-438, pass only
the target’s provision jobs; in
osac-operator/pkg/provisioning/provision_lifecycle_test.go lines 1201-1226,
update mockProvider.triggerDeprovisionFunc to accept and forward provisionJobs,
then assert each provider receives only its own jobs.
ori-amizur
reviewed
Aug 5, 2026
| log.Info("triggering deprovision job") | ||
| log.Info("triggering deprovision job", "target", target) | ||
|
|
||
| result, err := provider.TriggerDeprovision(ctx, resource, *jobs) |
Contributor
There was a problem hiding this comment.
Bug: unfiltered jobs passed to provider.
This passes the entire *jobs slice (all targets) to provider.TriggerDeprovision. The ProvisioningProvider.TriggerDeprovision interface takes provisionJobs []v1alpha1.JobStatus so providers can inspect provision job history (e.g., to cancel a running provision before deprovisioning). A fabric provider receiving k8s jobs — or vice versa — would see irrelevant history and could make wrong decisions based on it.
Suggestion: filter *jobs to only those matching target before passing:
targetJobs := filterJobsByTarget(*jobs, target)
result, err := provider.TriggerDeprovision(ctx, resource, targetJobs)The test mock currently discards the provisionJobs argument in triggerDeprovisionFunc, so this bug has no failing test. Consider widening the mock callback signature to accept and assert on provisionJobs.
| callbacks *PollCallbacks, | ||
| checkAPIServer func() bool, | ||
| ) (ctrl.Result, error, bool) { | ||
| action, latestJob := evaluateActionForTarget(provState, target, checkAPIServer) |
Contributor
There was a problem hiding this comment.
Nit: error should be the last return value.
runLifecycleCore returns (ctrl.Result, error, bool). Go convention (and revive's error-return rule) places error last. Since this function is unexported with only two call sites, swapping to (ctrl.Result, bool, error) is a contained change.
| // included here — a target that was never dispatched (e.g. a resource with | ||
| // no k8s manager configured) has no job history to evaluate and should be | ||
| // omitted by the caller rather than passed in. | ||
| func RunMultiTargetProvisioningLifecycle( |
Contributor
There was a problem hiding this comment.
Missing migration path for legacy untagged jobs.
Existing resources have jobs with Target: "". When a controller switches from RunProvisioningLifecycle to RunMultiTargetProvisioningLifecycle, those untagged jobs won't match any non-empty target name — FindLatestJobByTypeAndTarget(jobs, Provision, "fabric") returns nil for a job with Target: "". Each target will see no prior job history and trigger a fresh provision, potentially re-provisioning something that's already running.
The test suite starts from either empty jobs or already-tagged jobs; there's no test for the upgrade path. I'd suggest at minimum:
- Document the expected migration behavior (e.g., does the caller need to backfill
Targeton existing jobs before switching to multi-target? Or shouldevaluateActionForTargettreat untagged jobs as belonging to a designated legacy target?). - Add a test with pre-existing
Target: ""jobs to encode whichever strategy you choose.
| func CheckAPIServerForNonTerminalProvisionJobAndTarget(ctx context.Context, apiReader client.Reader, key client.ObjectKey, fresh client.Object, extract JobsExtractor, target string) bool { | ||
| log := ctrllog.FromContext(ctx) | ||
| if err := apiReader.Get(ctx, key, fresh); err != nil { | ||
| return false |
Contributor
There was a problem hiding this comment.
Silenced API server read error.
When apiReader.Get fails, this returns false (no non-terminal job found), which bypasses the duplicate-job guard and lets a new job be triggered. The error is silently discarded. Consider logging it so operators can diagnose why the guard was bypassed:
if err := apiReader.Get(ctx, key, fresh); err != nil {
log.Error(err, "failed to read resource from API server; proceeding without duplicate-job check", "target", target)
return false
}
danmanor
approved these changes
Aug 5, 2026
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, SiddarthR56 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This was referenced Aug 6, 2026
SiddarthR56
added a commit
to SiddarthR56/osac
that referenced
this pull request
Aug 10, 2026
Fixes findings from CodeRabbit and ori-amizur on PRs osac-project#225 and osac-project#147: - resolveImplementationStrategy now falls back to the k8s target for K8sFallback kinds instead of the legacy strategy. - SecurityGroupReconciler errors on an ambiguous parent VirtualNetwork lookup instead of silently picking one. - Subnet clears its stale k8s implementation-strategy annotation when a NetworkClass transitions from dual-dispatch back to fabric-only, and handleDeprovisioning guards against an empty fabricStrategy. - cmd/main.go reuses a single NetworkClassesClient instead of constructing a duplicate. - TriggerDeprovision now only receives the calling target's own provision jobs instead of every target's history. - Multi-target provisioning/deprovisioning backfills pre-existing untagged (Target == "") job history onto a designated target (JobTarget/DeprovisionTarget.AbsorbsLegacyHistory) so resources provisioned before dual-dispatch existed aren't re-provisioned or re-deprovisioned on upgrade. - runLifecycleCore now returns (Result, bool, error), matching Go's error-last convention. - CheckAPIServerForNonTerminalProvisionJobAndTarget logs API server read failures instead of silently bypassing the duplicate-job guard. Assisted-by: Cursor/Claude Co-authored-by: Cursor <cursoragent@cursor.com>
SiddarthR56
added a commit
to SiddarthR56/osac
that referenced
this pull request
Aug 12, 2026
Fixes findings from CodeRabbit and ori-amizur on PRs osac-project#225 and osac-project#147: - resolveImplementationStrategy now falls back to the k8s target for K8sFallback kinds instead of the legacy strategy. - SecurityGroupReconciler errors on an ambiguous parent VirtualNetwork lookup instead of silently picking one. - Subnet clears its stale k8s implementation-strategy annotation when a NetworkClass transitions from dual-dispatch back to fabric-only, and handleDeprovisioning guards against an empty fabricStrategy. - cmd/main.go reuses a single NetworkClassesClient instead of constructing a duplicate. - TriggerDeprovision now only receives the calling target's own provision jobs instead of every target's history. - Multi-target provisioning/deprovisioning backfills pre-existing untagged (Target == "") job history onto a designated target (JobTarget/DeprovisionTarget.AbsorbsLegacyHistory) so resources provisioned before dual-dispatch existed aren't re-provisioned or re-deprovisioned on upgrade. - runLifecycleCore now returns (Result, bool, error), matching Go's error-last convention. - CheckAPIServerForNonTerminalProvisionJobAndTarget logs API server read failures instead of silently bypassing the duplicate-job guard. Assisted-by: Cursor/Claude Co-authored-by: Cursor <cursoragent@cursor.com>
SiddarthR56
added a commit
to SiddarthR56/osac
that referenced
this pull request
Aug 13, 2026
Fixes findings from CodeRabbit and ori-amizur on PRs osac-project#225 and osac-project#147: - resolveImplementationStrategy now falls back to the k8s target for K8sFallback kinds instead of the legacy strategy. - SecurityGroupReconciler errors on an ambiguous parent VirtualNetwork lookup instead of silently picking one. - Subnet clears its stale k8s implementation-strategy annotation when a NetworkClass transitions from dual-dispatch back to fabric-only, and handleDeprovisioning guards against an empty fabricStrategy. - cmd/main.go reuses a single NetworkClassesClient instead of constructing a duplicate. - TriggerDeprovision now only receives the calling target's own provision jobs instead of every target's history. - Multi-target provisioning/deprovisioning backfills pre-existing untagged (Target == "") job history onto a designated target (JobTarget/DeprovisionTarget.AbsorbsLegacyHistory) so resources provisioned before dual-dispatch existed aren't re-provisioned or re-deprovisioned on upgrade. - runLifecycleCore now returns (Result, bool, error), matching Go's error-last convention. - CheckAPIServerForNonTerminalProvisionJobAndTarget logs API server read failures instead of silently bypassing the duplicate-job guard. Assisted-by: Cursor/Claude Co-authored-by: Cursor <cursoragent@cursor.com>
SiddarthR56
added a commit
to SiddarthR56/osac
that referenced
this pull request
Aug 13, 2026
Fixes findings from CodeRabbit and ori-amizur on PRs osac-project#225 and osac-project#147: - resolveImplementationStrategy now falls back to the k8s target for K8sFallback kinds instead of the legacy strategy. - SecurityGroupReconciler errors on an ambiguous parent VirtualNetwork lookup instead of silently picking one. - Subnet clears its stale k8s implementation-strategy annotation when a NetworkClass transitions from dual-dispatch back to fabric-only, and handleDeprovisioning guards against an empty fabricStrategy. - cmd/main.go reuses a single NetworkClassesClient instead of constructing a duplicate. - TriggerDeprovision now only receives the calling target's own provision jobs instead of every target's history. - Multi-target provisioning/deprovisioning backfills pre-existing untagged (Target == "") job history onto a designated target (JobTarget/DeprovisionTarget.AbsorbsLegacyHistory) so resources provisioned before dual-dispatch existed aren't re-provisioned or re-deprovisioned on upgrade. - runLifecycleCore now returns (Result, bool, error), matching Go's error-last convention. - CheckAPIServerForNonTerminalProvisionJobAndTarget logs API server read failures instead of silently bypassing the duplicate-job guard. Assisted-by: Cursor/Claude Co-authored-by: Cursor <cursoragent@cursor.com>
SiddarthR56
added a commit
to SiddarthR56/osac
that referenced
this pull request
Aug 17, 2026
Fixes findings from CodeRabbit and ori-amizur on PRs osac-project#225 and osac-project#147: - resolveImplementationStrategy now falls back to the k8s target for K8sFallback kinds instead of the legacy strategy. - SecurityGroupReconciler errors on an ambiguous parent VirtualNetwork lookup instead of silently picking one. - Subnet clears its stale k8s implementation-strategy annotation when a NetworkClass transitions from dual-dispatch back to fabric-only, and handleDeprovisioning guards against an empty fabricStrategy. - cmd/main.go reuses a single NetworkClassesClient instead of constructing a duplicate. - TriggerDeprovision now only receives the calling target's own provision jobs instead of every target's history. - Multi-target provisioning/deprovisioning backfills pre-existing untagged (Target == "") job history onto a designated target (JobTarget/DeprovisionTarget.AbsorbsLegacyHistory) so resources provisioned before dual-dispatch existed aren't re-provisioned or re-deprovisioned on upgrade. - runLifecycleCore now returns (Result, bool, error), matching Go's error-last convention. - CheckAPIServerForNonTerminalProvisionJobAndTarget logs API server read failures instead of silently bypassing the duplicate-job guard. Assisted-by: Cursor/Claude Co-authored-by: Cursor <cursoragent@cursor.com>
SiddarthR56
added a commit
to SiddarthR56/osac
that referenced
this pull request
Aug 18, 2026
Fixes findings from CodeRabbit and ori-amizur on PRs osac-project#225 and osac-project#147: - resolveImplementationStrategy now falls back to the k8s target for K8sFallback kinds instead of the legacy strategy. - SecurityGroupReconciler errors on an ambiguous parent VirtualNetwork lookup instead of silently picking one. - Subnet clears its stale k8s implementation-strategy annotation when a NetworkClass transitions from dual-dispatch back to fabric-only, and handleDeprovisioning guards against an empty fabricStrategy. - cmd/main.go reuses a single NetworkClassesClient instead of constructing a duplicate. - TriggerDeprovision now only receives the calling target's own provision jobs instead of every target's history. - Multi-target provisioning/deprovisioning backfills pre-existing untagged (Target == "") job history onto a designated target (JobTarget/DeprovisionTarget.AbsorbsLegacyHistory) so resources provisioned before dual-dispatch existed aren't re-provisioned or re-deprovisioned on upgrade. - runLifecycleCore now returns (Result, bool, error), matching Go's error-last convention. - CheckAPIServerForNonTerminalProvisionJobAndTarget logs API server read failures instead of silently bypassing the duplicate-job guard. Assisted-by: Cursor/Claude Co-authored-by: Cursor <cursoragent@cursor.com>
amito
pushed a commit
to amito/osac
that referenced
this pull request
Aug 19, 2026
Fixes findings from CodeRabbit and ori-amizur on PRs osac-project#225 and osac-project#147: - resolveImplementationStrategy now falls back to the k8s target for K8sFallback kinds instead of the legacy strategy. - SecurityGroupReconciler errors on an ambiguous parent VirtualNetwork lookup instead of silently picking one. - Subnet clears its stale k8s implementation-strategy annotation when a NetworkClass transitions from dual-dispatch back to fabric-only, and handleDeprovisioning guards against an empty fabricStrategy. - cmd/main.go reuses a single NetworkClassesClient instead of constructing a duplicate. - TriggerDeprovision now only receives the calling target's own provision jobs instead of every target's history. - Multi-target provisioning/deprovisioning backfills pre-existing untagged (Target == "") job history onto a designated target (JobTarget/DeprovisionTarget.AbsorbsLegacyHistory) so resources provisioned before dual-dispatch existed aren't re-provisioned or re-deprovisioned on upgrade. - runLifecycleCore now returns (Result, bool, error), matching Go's error-last convention. - CheckAPIServerForNonTerminalProvisionJobAndTarget logs API server read failures instead of silently bypassing the duplicate-job guard. Assisted-by: Cursor/Claude Co-authored-by: Cursor <cursoragent@cursor.com>
eliorerz
pushed a commit
that referenced
this pull request
Sep 18, 2026
…ext keys, render version load error Per rawagner review on PR #147: - Remove trim() on cluster version id (useClusterVersion) and on versionName in the create payload; both come from trusted sources (backend id, dropdown selection) that never contain whitespace. - Rename the version query destructure to isLoading/error and render the actual error message via getErrorMessage in the load-error Alert. - Replace dotted i18n keys with the English-text keys used across the codebase (Select a version, Could not load cluster versions, Retry, This version is deprecated ...). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
eliorerz
pushed a commit
that referenced
this pull request
Sep 18, 2026
…rsion-wizard OSAC-2169: [UI] Implement version selection in cluster creation wizard
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Validation