Add prompt-on-launch support for workflow jobs#183
Open
PabloHiro wants to merge 1 commit intoansible:mainfrom
Open
Add prompt-on-launch support for workflow jobs#183PabloHiro wants to merge 1 commit intoansible:mainfrom
PabloHiro wants to merge 1 commit intoansible:mainfrom
Conversation
PabloHiro
force-pushed
the
feature/support-for-prompt-on-launch-workflow-job
branch
from
c0a8be3 to
436444b
Compare
Contributor
Author
|
Acceptance tests: Details |
|
davemulford
requested changes
Feb 11, 2026
Contributor
davemulford
left a comment
davemulford
left a comment
There was a problem hiding this comment.
Request changes to give time for docs coordination.
📝 WalkthroughWalkthroughAdds prompt-on-launch support for workflow jobs: new schema fields ( Changes
Sequence DiagramssequenceDiagram
participant Client as Client (Terraform)
participant Provider as Provider
participant AAP as AAP API
Client->>Provider: Request launch (aap_workflow_job / action)
Provider->>AAP: GET /.../{id}/launch/
AAP-->>Provider: Launch config (ask_on_launch flags, ignored_fields, etc.)
Provider->>Provider: CanWorkflowJobBeLaunched() — validate required prompt fields
alt All required fields present
Provider->>AAP: POST /.../{id}/launch/ with payload (extra_vars, job_tags, labels, limit, skip_tags, inventory_id...)
AAP-->>Provider: 201 Created + Job response
Provider->>Provider: WaitForWorkflowJobCompletion() (if requested) — poll status
AAP-->>Provider: Job status updates
Provider-->>Client: Success / state updated
else Missing required fields
Provider-->>Client: Error (Missing required field)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/provider/workflow_job_launch_action.go (1)
123-132: Nit:labelsmissing from debug log while other new fields are logged.For symmetry with the newly added
limit,job_tags, andskip_tags, consider includinglabelsin the debug map so the full set of prompt-on-launch inputs is captured.Proposed tweak
tflog.Debug(ctx, "workflow job launched", map[string]interface{}{ "url": jobResponse.URL, "status": jobResponse.Status, "template_id": jobResponse.TemplateID, "inventory_id": jobResponse.Inventory, "extra_vars": jobResponse.ExtraVars, "limit": jobResponse.Limit, "job_tags": jobResponse.JobTags, "skip_tags": jobResponse.SkipTags, + "labels": jobResponse.Labels, })Also, the removal of
ignored_fieldsfrom this map isn't mentioned in the PR description — confirm that's intentional, as it was useful for surfacing AAP-side rejected inputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/workflow_job_launch_action.go` around lines 123 - 132, The debug log in tflog.Debug within workflow_job_launch_action.go is missing the labels field from jobResponse; update the map passed to tflog.Debug (the one currently logging "url","status","template_id","inventory_id","extra_vars","limit","job_tags","skip_tags") to include "labels": jobResponse.Labels so prompt-on-launch inputs are fully captured, and verify whether the previously-logged ignored_fields was intentionally removed—if it should remain, re-add "ignored_fields": jobResponse.IgnoredFields to the same map.internal/provider/workflow_job_resource_test.go (1)
1111-1115: Optional:testAccCheckWorkflowJobPauseis redundant whenwait_for_completion = true.The config sets
wait_for_completion = true, so the workflow job is already in a final state by the timeCheckruns. The explicit pause is harmless but can be dropped to keep the test focused and faster.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/workflow_job_resource_test.go` around lines 1111 - 1115, The test config uses wait_for_completion = true via testAccWorkflowJobAllFieldsOnPrompt so the job is already final when checks run; remove the redundant check testAccCheckWorkflowJobPause from the resource.ComposeAggregateTestCheckFunc in the failing test (the reference to testAccWorkflowJobAllFieldsOnPrompt, testAccCheckWorkflowJobPause and the ComposeAggregateTestCheckFunc identify the spot) to simplify and speed the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/provider/workflow_job_resource.go`:
- Around line 492-518: The validation incorrectly treats Terraform "unknown"
plan values as provided because it only checks IsNull(); update the logic around
the validations slice (the block building validations and the loop over it) to
consider IsUnknown() for the Optional+Computed fields (r.JobTags, r.SkipTags,
r.Limit, r.InventoryID) by computing a boolean like isNullOrUnknown (e.g.,
isNull || IsUnknown) and use that instead of v.isNull for the required-field
error and the "will be ignored" warning; keep ExtraVars and Labels behavior
unchanged. Also add unit tests that supply Unknown plan values for those four
Optional+Computed fields to assert the Create-path validation produces the
expected error/warning behavior.
---
Nitpick comments:
In `@internal/provider/workflow_job_launch_action.go`:
- Around line 123-132: The debug log in tflog.Debug within
workflow_job_launch_action.go is missing the labels field from jobResponse;
update the map passed to tflog.Debug (the one currently logging
"url","status","template_id","inventory_id","extra_vars","limit","job_tags","skip_tags")
to include "labels": jobResponse.Labels so prompt-on-launch inputs are fully
captured, and verify whether the previously-logged ignored_fields was
intentionally removed—if it should remain, re-add "ignored_fields":
jobResponse.IgnoredFields to the same map.
In `@internal/provider/workflow_job_resource_test.go`:
- Around line 1111-1115: The test config uses wait_for_completion = true via
testAccWorkflowJobAllFieldsOnPrompt so the job is already final when checks run;
remove the redundant check testAccCheckWorkflowJobPause from the
resource.ComposeAggregateTestCheckFunc in the failing test (the reference to
testAccWorkflowJobAllFieldsOnPrompt, testAccCheckWorkflowJobPause and the
ComposeAggregateTestCheckFunc identify the spot) to simplify and speed the test.
🪄 Autofix (Beta)
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: Pro Plus
Run ID: 7c4f89d2-7a2e-4beb-a73c-7df9ffb963e0
📒 Files selected for processing (9)
changelogs/fragments/20260122-prompt-on-launch-support-workflow-jobs.ymldocs/actions/workflow_job_launch.mddocs/resources/workflow_job.mdinternal/provider/workflow_job_launch_action.gointernal/provider/workflow_job_launch_action_test.gointernal/provider/workflow_job_resource.gointernal/provider/workflow_job_resource_test.gotesting/playbook.ymltesting/templates/acceptance_test_vars.env.j2
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/provider/job_resource.go (1)
654-669:⚠️ Potential issue | 🟠 MajorUnknown plan values bypass required-field checks and trigger spurious warnings on Create.
On
Create,Limit,JobTags,SkipTags,InventoryID,DiffMode,ExecutionEnvironmentID,Forks,Verbosity,InstanceGroups,Timeout, andJobSliceCountare allOptional+Computed. When the user omits them there is no prior state forUseStateForUnknown()to substitute, so they arrive as unknown, not null. SinceIsNull()returnsfalsefor unknown values:
- When the template sets e.g.
ask_limit_on_launch=trueand the user omitslimit, the required-field validation is skipped and the POST proceeds with an empty value, producing an API error rather than the intended provider diagnostic.- When the template does not ask for a field and the user also omitted it, the "field will be ignored" warning fires spuriously on every Create.
ExtraVars(Optional only) andCredentials/Labels(WriteOnly) are unaffected. Existing unit tests use explicit Null/Value and do not catch this.🛠️ Proposed fix
validations := []LaunchFieldValidation{ {launchConfig.AskVariablesOnLaunch, r.ExtraVars.IsNull(), "extra_vars"}, - {launchConfig.AskTagsOnLaunch, r.JobTags.IsNull(), "job_tags"}, - {launchConfig.AskSkipTagsOnLaunch, r.SkipTags.IsNull(), "skip_tags"}, - {launchConfig.AskDiffModeOnLaunch, r.DiffMode.IsNull(), "diff_mode"}, - {launchConfig.AskLimitOnLaunch, r.Limit.IsNull(), "limit"}, - {launchConfig.AskInventoryOnLaunch, r.InventoryID.IsNull(), "inventory_id"}, + {launchConfig.AskTagsOnLaunch, r.JobTags.IsNull() || r.JobTags.IsUnknown(), "job_tags"}, + {launchConfig.AskSkipTagsOnLaunch, r.SkipTags.IsNull() || r.SkipTags.IsUnknown(), "skip_tags"}, + {launchConfig.AskDiffModeOnLaunch, r.DiffMode.IsNull() || r.DiffMode.IsUnknown(), "diff_mode"}, + {launchConfig.AskLimitOnLaunch, r.Limit.IsNull() || r.Limit.IsUnknown(), "limit"}, + {launchConfig.AskInventoryOnLaunch, r.InventoryID.IsNull() || r.InventoryID.IsUnknown(), "inventory_id"}, {launchConfig.AskCredentialOnLaunch, r.Credentials.IsNull(), "credentials"}, - {launchConfig.AskExecutionEnvironmentOnLaunch, r.ExecutionEnvironmentID.IsNull(), "execution_environment"}, + {launchConfig.AskExecutionEnvironmentOnLaunch, r.ExecutionEnvironmentID.IsNull() || r.ExecutionEnvironmentID.IsUnknown(), "execution_environment"}, {launchConfig.AskLabelsOnLaunch, r.Labels.IsNull(), "labels"}, - {launchConfig.AskForksOnLaunch, r.Forks.IsNull(), "forks"}, - {launchConfig.AskVerbosityOnLaunch, r.Verbosity.IsNull(), "verbosity"}, - {launchConfig.AskInstanceGroupsOnLaunch, r.InstanceGroups.IsNull(), "instance_groups"}, - {launchConfig.AskTimeoutOnLaunch, r.Timeout.IsNull(), "timeout"}, - {launchConfig.AskJobSliceCountOnLaunch, r.JobSliceCount.IsNull(), "job_slice_count"}, + {launchConfig.AskForksOnLaunch, r.Forks.IsNull() || r.Forks.IsUnknown(), "forks"}, + {launchConfig.AskVerbosityOnLaunch, r.Verbosity.IsNull() || r.Verbosity.IsUnknown(), "verbosity"}, + {launchConfig.AskInstanceGroupsOnLaunch, r.InstanceGroups.IsNull() || r.InstanceGroups.IsUnknown(), "instance_groups"}, + {launchConfig.AskTimeoutOnLaunch, r.Timeout.IsNull() || r.Timeout.IsUnknown(), "timeout"}, + {launchConfig.AskJobSliceCountOnLaunch, r.JobSliceCount.IsNull() || r.JobSliceCount.IsUnknown(), "job_slice_count"}, }Alternatively, consider folding the null-or-unknown check into
LaunchFieldValidation(e.g., accept anattr.Valueand computeIsNull() || IsUnknown()centrally) to prevent this from recurring, and extend unit tests with Unknown values to cover the Create path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/job_resource.go` around lines 654 - 669, The validation loop builds LaunchFieldValidation entries using expressions like launchConfig.AskLimitOnLaunch and r.Limit.IsNull(), but attr.Values that are unknown on Create (Optional+Computed fields such as Limit, JobTags, SkipTags, InventoryID, DiffMode, ExecutionEnvironmentID, Forks, Verbosity, InstanceGroups, Timeout, JobSliceCount) return IsNull() == false and thus bypass required checks and emit spurious warnings; fix by treating unknown the same as null—either change each check to r.<Field>.IsNull() || r.<Field>.IsUnknown() for the listed fields in the validations slice, or better refactor LaunchFieldValidation to accept the attr.Value and compute (v.IsNull() || v.IsUnknown()) centrally so the required/ignored logic uses that combined null-or-unknown predicate (update unit tests to include Unknown values on Create).
♻️ Duplicate comments (1)
internal/provider/workflow_job_resource.go (1)
472-482:⚠️ Potential issue | 🟠 MajorUnknown plan values bypass required-field checks on Create (same class of bug as in
job_resource.go).
Limit,JobTags,SkipTags, andInventoryIDareOptional+Computed, so on Create they arrive as unknown (not null) when the user omits them —UseStateForUnknown()has no prior state to substitute.IsNull()isfalsefor unknown values, so:
- Required-on-launch fields silently skip the error and the POST proceeds with empty values, producing API errors instead of the intended "Missing required field" diagnostic.
- For fields the template does not ask for, the "field will be ignored" warning fires spuriously on every Create.
ExtraVars(Optional only) andLabels(Optional+WriteOnly) resolve to null when unset and are unaffected.🛠️ Proposed fix
validations := []LaunchFieldValidation{ {launchConfig.AskVariablesOnLaunch, r.ExtraVars.IsNull(), "extra_vars"}, - {launchConfig.AskTagsOnLaunch, r.JobTags.IsNull(), "job_tags"}, - {launchConfig.AskSkipTagsOnLaunch, r.SkipTags.IsNull(), "skip_tags"}, - {launchConfig.AskLimitOnLaunch, r.Limit.IsNull(), "limit"}, - {launchConfig.AskInventoryOnLaunch, r.InventoryID.IsNull(), "inventory_id"}, + {launchConfig.AskTagsOnLaunch, r.JobTags.IsNull() || r.JobTags.IsUnknown(), "job_tags"}, + {launchConfig.AskSkipTagsOnLaunch, r.SkipTags.IsNull() || r.SkipTags.IsUnknown(), "skip_tags"}, + {launchConfig.AskLimitOnLaunch, r.Limit.IsNull() || r.Limit.IsUnknown(), "limit"}, + {launchConfig.AskInventoryOnLaunch, r.InventoryID.IsNull() || r.InventoryID.IsUnknown(), "inventory_id"}, {launchConfig.AskLabelsOnLaunch, r.Labels.IsNull(), "labels"}, }Or, preferably, update
LaunchFieldValidationto accept theattr.Valueand computeIsNull() || IsUnknown()in one place (see comment onutils.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/workflow_job_resource.go` around lines 472 - 482, The validations currently call LaunchFieldValidation with boolean checks using r.Limit.IsNull(), r.JobTags.IsNull(), r.SkipTags.IsNull(), r.InventoryID.IsNull(), which miss Unknown values on Create (Optional+Computed) and let required-on-launch fields bypass diagnostics; update the validation logic so the check treats unknown the same as null by using IsNull() || IsUnknown() for these attributes, or better, modify LaunchFieldValidation to accept the attr.Value (e.g., r.Limit, r.JobTags, r.SkipTags, r.InventoryID) and perform the combined IsNull() || IsUnknown() test inside ValidateLaunchFields/LaunchFieldValidation so unknowns trigger the required-field errors and suppress spurious "will be ignored" warnings on create.
🧹 Nitpick comments (3)
internal/provider/utils.go (1)
130-158: Design makes the null-vs-unknown pitfall easy to reintroduce.
LaunchFieldValidation.IsNullis a precomputed bool supplied by callers, which is exactly why thejob_resource.goandworkflow_job_resource.govalidation sites only checkedIsNull()and missedIsUnknown()(see related comments). Consider changing the field to accept theattr.Valuedirectly and derive the check centrally:type LaunchFieldValidation struct { AskOnLaunch bool Value attr.Value FieldName string } // inside ValidateLaunchFields missing := v.Value.IsNull() || v.Value.IsUnknown()This guarantees both Null and Unknown are treated as "not provided" across every call site and future additions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/utils.go` around lines 130 - 158, The validation currently relies on a precomputed boolean IsNull on LaunchFieldValidation which caused callers (job_resource.go, workflow_job_resource.go) to miss IsUnknown cases; change LaunchFieldValidation to carry the actual attr.Value (rename IsNull -> Value attr.Value) and update ValidateLaunchFields to compute "missing" as v.Value.IsNull() || v.Value.IsUnknown() so both null and unknown are treated as not provided; update all call sites that construct LaunchFieldValidation to pass the attr.Value instead of IsNull and adjust checks that previously used IsNull to rely on ValidateLaunchFields' derived missing logic.internal/provider/workflow_job_resource_test.go (1)
518-659: Tests only exercise Null/Value; add Unknown coverage.
CanWorkflowJobBeLaunchedunit tests cover explicitNullandValuestates but neverUnknown. Since Optional+Computed fields arrive asUnknownon Create, adding cases likeLimit: customtypes.NewAAPCustomStringUnknown()withAskLimitOnLaunch: truewould catch the null-vs-unknown bug that currently surfaces only in real Terraform runs. See the related comment onCanWorkflowJobBeLaunched.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/workflow_job_resource_test.go` around lines 518 - 659, The tests in TestWorkflowJobModelCanWorkflowJobBeLaunched only exercise Null and Value states; add test cases that set Optional+Computed fields to Unknown to match Create-time Terraform behavior so CanWorkflowJobBeLaunched handles Unknown correctly. Specifically, in that table-driven test add rows where fields such as Limit, ExtraVars, JobTags, SkipTags use customtypes.NewAAPCustomStringUnknown(), InventoryID uses types.Int64Unknown(), and Labels uses types.ListUnknown(types.Int64Type) while the corresponding WorkflowJobLaunchAPIModel flags (e.g., AskLimitOnLaunch, AskVariablesOnLaunch, AskInventoryOnLaunch, AskLabelsOnLaunch) are true; assert expectError/expectWarnings accordingly to catch the null-vs-unknown bug when calling WorkflowJobModel.CanWorkflowJobBeLaunched.internal/provider/job_resource.go (1)
686-721: Consider extracting aWaitForJobCompletionhelper for parity with the workflow job resource.
WorkflowJobResource.launchAndWaitdelegates polling toWorkflowJobModel.WaitForWorkflowJobCompletion, whileJobResource.launchAndWaitinlines theretry.RetryContextcall. Mirroring the helper onJobModelwould make the two resources symmetric and easier to maintain/test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/provider/job_resource.go`:
- Around line 654-669: The validation loop builds LaunchFieldValidation entries
using expressions like launchConfig.AskLimitOnLaunch and r.Limit.IsNull(), but
attr.Values that are unknown on Create (Optional+Computed fields such as Limit,
JobTags, SkipTags, InventoryID, DiffMode, ExecutionEnvironmentID, Forks,
Verbosity, InstanceGroups, Timeout, JobSliceCount) return IsNull() == false and
thus bypass required checks and emit spurious warnings; fix by treating unknown
the same as null—either change each check to r.<Field>.IsNull() ||
r.<Field>.IsUnknown() for the listed fields in the validations slice, or better
refactor LaunchFieldValidation to accept the attr.Value and compute (v.IsNull()
|| v.IsUnknown()) centrally so the required/ignored logic uses that combined
null-or-unknown predicate (update unit tests to include Unknown values on
Create).
---
Duplicate comments:
In `@internal/provider/workflow_job_resource.go`:
- Around line 472-482: The validations currently call LaunchFieldValidation with
boolean checks using r.Limit.IsNull(), r.JobTags.IsNull(), r.SkipTags.IsNull(),
r.InventoryID.IsNull(), which miss Unknown values on Create (Optional+Computed)
and let required-on-launch fields bypass diagnostics; update the validation
logic so the check treats unknown the same as null by using IsNull() ||
IsUnknown() for these attributes, or better, modify LaunchFieldValidation to
accept the attr.Value (e.g., r.Limit, r.JobTags, r.SkipTags, r.InventoryID) and
perform the combined IsNull() || IsUnknown() test inside
ValidateLaunchFields/LaunchFieldValidation so unknowns trigger the
required-field errors and suppress spurious "will be ignored" warnings on
create.
---
Nitpick comments:
In `@internal/provider/utils.go`:
- Around line 130-158: The validation currently relies on a precomputed boolean
IsNull on LaunchFieldValidation which caused callers (job_resource.go,
workflow_job_resource.go) to miss IsUnknown cases; change LaunchFieldValidation
to carry the actual attr.Value (rename IsNull -> Value attr.Value) and update
ValidateLaunchFields to compute "missing" as v.Value.IsNull() ||
v.Value.IsUnknown() so both null and unknown are treated as not provided; update
all call sites that construct LaunchFieldValidation to pass the attr.Value
instead of IsNull and adjust checks that previously used IsNull to rely on
ValidateLaunchFields' derived missing logic.
In `@internal/provider/workflow_job_resource_test.go`:
- Around line 518-659: The tests in TestWorkflowJobModelCanWorkflowJobBeLaunched
only exercise Null and Value states; add test cases that set Optional+Computed
fields to Unknown to match Create-time Terraform behavior so
CanWorkflowJobBeLaunched handles Unknown correctly. Specifically, in that
table-driven test add rows where fields such as Limit, ExtraVars, JobTags,
SkipTags use customtypes.NewAAPCustomStringUnknown(), InventoryID uses
types.Int64Unknown(), and Labels uses types.ListUnknown(types.Int64Type) while
the corresponding WorkflowJobLaunchAPIModel flags (e.g., AskLimitOnLaunch,
AskVariablesOnLaunch, AskInventoryOnLaunch, AskLabelsOnLaunch) are true; assert
expectError/expectWarnings accordingly to catch the null-vs-unknown bug when
calling WorkflowJobModel.CanWorkflowJobBeLaunched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 00779f03-0761-4ee3-8b18-e136b2c80d91
📒 Files selected for processing (6)
internal/provider/job_resource.gointernal/provider/utils.gointernal/provider/workflow_job_launch_action.gointernal/provider/workflow_job_launch_action_test.gointernal/provider/workflow_job_resource.gointernal/provider/workflow_job_resource_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/provider/workflow_job_launch_action.go
melissalkelly
force-pushed
the
feature/support-for-prompt-on-launch-workflow-job
branch
from
b166c69 to
f833543
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/provider/workflow_job_resource_test.go (1)
518-659: Consider adding anIsUnknown()case for Optional+Computed fields.
TestWorkflowJobModelCanWorkflowJobBeLaunchedonly exercises Null vs. Value forLimit/JobTags/SkipTags/InventoryID, but on Create these Optional+Computed attributes arrive as unknown when unset. The sharedValidateLaunchFieldsalready handles both (IsNull() || IsUnknown()), so adding a couple of Unknown-based cases would lock in that contract from the call site and catch future regressions if someone refactors the validator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/workflow_job_resource_test.go` around lines 518 - 659, TestWorkflowJobModelCanWorkflowJobBeLaunched currently only tests Null vs Value for optional+computed fields; add cases where those fields are Unknown (IsUnknown) to ensure Optional+Computed attributes behave the same as Null at the call site—update the test table to include at least two test cases (e.g., one where Limit/JobTags/SkipTags are Unknown and one where InventoryID/ExtraVars/Labels are Unknown) and assert the same expectError/expectWarnings outcomes as their Null counterparts; reference the test function TestWorkflowJobModelCanWorkflowJobBeLaunched and the validator ValidateLaunchFields to mirror its IsNull() || IsUnknown() logic.internal/provider/job_resource.go (1)
669-704: Consider extracting a sharedWaitForJobCompletionhelper for parity withWorkflowJobModel.
WorkflowJobModel.WaitForWorkflowJobCompletionencapsulates the timeout/retry/diagnostic pattern, whileJobResource.launchAndWaitstill inlines the same logic here. Factoring an analogousJobModel.WaitForJobCompletion(or a single shared helper keyed offURL/timeout) would keep the two resources behaviorally aligned and shrink drift risk when the wait semantics evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/job_resource.go` around lines 669 - 704, Extract the inline wait/retry/timeout logic from JobResource.launchAndWait into a reusable helper (e.g., JobModel.WaitForJobCompletion or a shared WaitForJobCompletion that accepts ctx, client, url, timeout, and a progress callback) mirroring the pattern used by WorkflowJobModel.WaitForWorkflowJobCompletion; replace the inlined block that builds timeout, retryProgressFunc, calls retry.RetryContext with retryUntilAAPJobReachesAnyFinalState, and sets data.Status with a call to the new helper, ensuring diagnostics returned on error are preserved and that LaunchJobWithResponse remains invoked before waiting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/provider/utils.go`:
- Around line 161-186: The ParseIgnoredFieldsToList function builds keysList by
iterating a map which yields non-deterministic order; to avoid perpetual diffs
you should collect the map keys (or the mapped values via keyMapping) into a
slice, sort that slice using sort.Strings, then build keysList in that
deterministic order before calling types.ListValue; update
ParseIgnoredFieldsToList to perform the sort and add the "sort" import,
referencing the existing ParseIgnoredFieldsToList function, the keysList
variable and the keyMapping lookup.
---
Nitpick comments:
In `@internal/provider/job_resource.go`:
- Around line 669-704: Extract the inline wait/retry/timeout logic from
JobResource.launchAndWait into a reusable helper (e.g.,
JobModel.WaitForJobCompletion or a shared WaitForJobCompletion that accepts ctx,
client, url, timeout, and a progress callback) mirroring the pattern used by
WorkflowJobModel.WaitForWorkflowJobCompletion; replace the inlined block that
builds timeout, retryProgressFunc, calls retry.RetryContext with
retryUntilAAPJobReachesAnyFinalState, and sets data.Status with a call to the
new helper, ensuring diagnostics returned on error are preserved and that
LaunchJobWithResponse remains invoked before waiting.
In `@internal/provider/workflow_job_resource_test.go`:
- Around line 518-659: TestWorkflowJobModelCanWorkflowJobBeLaunched currently
only tests Null vs Value for optional+computed fields; add cases where those
fields are Unknown (IsUnknown) to ensure Optional+Computed attributes behave the
same as Null at the call site—update the test table to include at least two test
cases (e.g., one where Limit/JobTags/SkipTags are Unknown and one where
InventoryID/ExtraVars/Labels are Unknown) and assert the same
expectError/expectWarnings outcomes as their Null counterparts; reference the
test function TestWorkflowJobModelCanWorkflowJobBeLaunched and the validator
ValidateLaunchFields to mirror its IsNull() || IsUnknown() logic.
🪄 Autofix (Beta)
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: Pro Plus
Run ID: 4fccd57e-b555-41a6-a73a-aebc6c721b32
📒 Files selected for processing (5)
internal/provider/job_resource.gointernal/provider/utils.gointernal/provider/workflow_job_launch_action_test.gointernal/provider/workflow_job_resource.gointernal/provider/workflow_job_resource_test.go
Add support for limit, job_tags, skip_tags, and labels fields in workflow job resources and actions. These fields can be provided at launch time when the workflow job template allows it. Changes: - Add new optional fields to workflow_job resource and action - Validate fields against template's ask_on_launch settings - Add comprehensive unit tests for new fields - Refactor duplicated code into shared helpers - Fix validation to handle unknown values on Create - Add GetLaunchConfiguration and CommonJobSchemaAttributes helpers Co-authored-by: melissalkelly <melissalkelly1@gmail.com> Signed-off-by: melissalkelly <melissalkelly1@gmail.com>
melissalkelly
force-pushed
the
feature/support-for-prompt-on-launch-workflow-job
branch
from
5b67f4d to
9a79cec
Compare
|
melissalkelly
approved these changes
Apr 17, 2026
davemulford
approved these changes
Apr 17, 2026
melissalkelly
changed the title
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
4 participants
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.
Description
Added prompt-on-launch support for workflow jobs (aap_workflow_job resource and aap_workflow_job_launch action):
These fields were already added to regular job templates in PR #178. This PR completes the feature by adding them to workflow job templates.
Also includes refactoring to improve code quality:
Related Issue: #130 / AAP-51746
Type of Change
Testing Checklist
Unit Tests
make test)make testcov)Acceptance Tests
Code Quality
make lint)Documentation
make generatedocs)Testing Instructions
Run unit and acceptance tests. Check if the added scenarios make sense.
Changelog
changelogs/fragments/(if applicable)YYYYMMDD-description.ymlAdditional Notes
AI Attribution
Assisted-by: Claude Opus 4.5
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests
Chores