OSAC-874: Remove EDA provider and webhook client from osac-operator - #272
Conversation
|
@tchughesiv: This pull request references OSAC-874 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. |
|
Skipping CI for Draft Pull Request. |
|
Warning Review limit reached
More reviews will be available in 11 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (45)
WalkthroughThis PR removes EDA/webhook provisioning and standardizes controllers, wiring, charts, CRDs, tests, and docs on direct AAP REST API provisioning. Risk: Medium — controller wiring and deprovision timing changed; verify AAP env, templates, and migration of webhook-based deployments. ChangesEDA Removal and AAP Standardization
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Sequence Diagram (high-level provisioning flow) sequenceDiagram
participant Controller
participant AAPClient
participant KubernetesAPI
Controller->>AAPClient: TriggerProvision / launch job template
AAPClient-->>Controller: jobID
Controller->>KubernetesAPI: update CR status with jobID
Controller->>AAPClient: RunProvisioningLifecycle / GetJob (poll)
AAPClient-->>Controller: job status (running/failed/success)
Controller->>KubernetesAPI: update CR status/conditions on poll result
Note right of Controller: OnFailed callback -> set Failed if no VM reference yet
Security-focused risk summary: Risk severity — Medium. Impact: controller wiring and deprovision timing changes could cause delayed or skipped cleanup of cloud resources if AAP credentials/templates are misconfigured; verify AAP credentials, template names, and that deployments migrated from webhook/EDA are reconfigured to use AAP env vars.
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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 |
50d1f27 to
b840580
Compare
b840580 to
be521ed
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/main.go (1)
407-417: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRisk: Inconsistent AAP credential validation may cause deferred runtime failures.
setupTenantControllerexplicitly checksaapURL != "" && aapToken != ""before creating a provider (allowing graceful no-op mode), butsetupNetworkingControllerscreatesaapClientandnetworkingProviderunconditionally. If AAP credentials are missing, networking controllers will start successfully but fail at runtime when provisioning is attempted.Severity: Low — No security vulnerability, but operational reliability is affected. Misconfiguration manifests as cryptic API errors during reconciliation rather than clear startup validation.
Consider harmonizing the validation pattern: either validate early and fail fast for all controllers, or document that networking controllers require AAP configuration while tenant can operate without it.
🔧 Optional: Add early validation for networking AAP configuration
// Get provider configuration aapURL := os.Getenv(envAAPURL) aapToken := os.Getenv(envAAPToken) + if aapURL == "" || aapToken == "" { + return fmt.Errorf("networking controllers require AAP configuration: %s and %s must be set", envAAPURL, envAAPToken) + } aapInsecureSkipVerify := helpers.GetEnvWithDefault(envAAPInsecureSkipVerify, 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 `@cmd/main.go` around lines 407 - 417, The networking setup creates aapClient and networkingProvider unconditionally (call sites: aap.NewClient and provisioning.NewAAPProviderWithPrefix in setupNetworkingControllers) while setupTenantController guards on aapURL != "" && aapToken != ""; update setupNetworkingControllers to check the AAP env vars (aapURL and aapToken) before creating the client/provider and either (a) skip creating the provider and initialize networking controllers in no-op mode when creds are missing or (b) return an error to fail fast—mirror the credential check used by setupTenantController so runtime reconciliations don’t fail unexpectedly.
🤖 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 @.claude/rules/configuration.md:
- Around line 5-6: Add a blank line immediately after the heading "## AAP
Provisioning" so the heading is followed by an empty line before the list item
`OSAC_AAP_URL`; edit the block containing the "## AAP Provisioning" heading and
insert one newline to satisfy MD022 and prevent markdown lint/CI failures.
---
Outside diff comments:
In `@cmd/main.go`:
- Around line 407-417: The networking setup creates aapClient and
networkingProvider unconditionally (call sites: aap.NewClient and
provisioning.NewAAPProviderWithPrefix in setupNetworkingControllers) while
setupTenantController guards on aapURL != "" && aapToken != ""; update
setupNetworkingControllers to check the AAP env vars (aapURL and aapToken)
before creating the client/provider and either (a) skip creating the provider
and initialize networking controllers in no-op mode when creds are missing or
(b) return an error to fail fast—mirror the credential check used by
setupTenantController so runtime reconciliations don’t fail unexpectedly.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 663c6613-6876-4c1f-8354-c57fbd797bde
📒 Files selected for processing (44)
.claude/rules/configuration.mdAGENTS.mdREADME.mdapi/v1alpha1/job_types.gocharts/operator-crds/templates/osac.openshift.io_clusterorders.yamlcharts/operator-crds/templates/osac.openshift.io_computeinstances.yamlcharts/operator-crds/templates/osac.openshift.io_publicipattachments.yamlcharts/operator-crds/templates/osac.openshift.io_publicippools.yamlcharts/operator-crds/templates/osac.openshift.io_publicips.yamlcharts/operator-crds/templates/osac.openshift.io_securitygroups.yamlcharts/operator-crds/templates/osac.openshift.io_subnets.yamlcharts/operator-crds/templates/osac.openshift.io_tenants.yamlcharts/operator-crds/templates/osac.openshift.io_virtualnetworks.yamlcharts/operator/templates/deployment.yamlcharts/operator/values.yamlcmd/main.goconfig/crd/bases/osac.openshift.io_clusterorders.yamlconfig/crd/bases/osac.openshift.io_computeinstances.yamlconfig/crd/bases/osac.openshift.io_publicipattachments.yamlconfig/crd/bases/osac.openshift.io_publicippools.yamlconfig/crd/bases/osac.openshift.io_publicips.yamlconfig/crd/bases/osac.openshift.io_securitygroups.yamlconfig/crd/bases/osac.openshift.io_subnets.yamlconfig/crd/bases/osac.openshift.io_tenants.yamlconfig/crd/bases/osac.openshift.io_virtualnetworks.yamlconfig/manager/manager.yamlconfig/samples/osac-config-secret.yamlinternal/controller/clusterorder_controller_test.gointernal/controller/computeinstance_controller.gointernal/controller/computeinstance_controller_test.gointernal/controller/computeinstance_integration_test.gointernal/controller/computeinstance_provisioning_test.gointernal/controller/suite_test.gointernal/controller/webhook_common.gointernal/controller/webhook_common_test.gointernal/webhook/types.gopkg/provisioning/aap_provider.gopkg/provisioning/aap_provider_test.gopkg/provisioning/eda_provider.gopkg/provisioning/eda_provider_test.gopkg/provisioning/factory.gopkg/provisioning/factory_test.gopkg/provisioning/provider.gopkg/provisioning/provision_lifecycle.go
💤 Files with no reviewable changes (10)
- internal/controller/webhook_common_test.go
- charts/operator/values.yaml
- internal/controller/webhook_common.go
- pkg/provisioning/eda_provider_test.go
- internal/webhook/types.go
- pkg/provisioning/eda_provider.go
- config/manager/manager.yaml
- pkg/provisioning/provision_lifecycle.go
- internal/controller/computeinstance_controller.go
- charts/operator/templates/deployment.yaml
|
Thanks — valid observation, but deferring to a follow-up outside OSAC-874 scope. The tenant controller’s optional AAP guard is intentional: tenant storage provisioning can run in no-op mode when AAP templates aren’t configured, while the tenant reconciler still manages namespace/UDN lifecycle. Cluster, compute, and networking controllers all assume AAP is configured in production (installer overlays always set Happy to track that as a follow-up if we want consistent startup validation everywhere. |
|
/retest |
|
/retest |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/retest-required |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/main.go (1)
406-432:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor: networking controllers no longer fail fast on missing AAP config.
Lines 406-417 now build the shared networking provider through
aap.NewClient(...)+provisioning.NewAAPProviderWithPrefix(...), which skips the validating path used bycreateAAPProvider(...). IfOSAC_AAP_URLorOSAC_AAP_TOKENis empty/bad, the manager can still start and every networking reconcile fails later instead of aborting at startup. That widens the blast radius of a bad deployment and makes the misconfiguration much harder to detect.Suggested fix
aapURL := os.Getenv(envAAPURL) aapToken := os.Getenv(envAAPToken) + if aapURL == "" || aapToken == "" { + return fmt.Errorf("%s and %s must be set when networking controllers are enabled", envAAPURL, envAAPToken) + } aapInsecureSkipVerify := helpers.GetEnvWithDefault(envAAPInsecureSkipVerify, false) statusPollInterval := helpers.GetEnvWithDefault(envAAPStatusPollInterval, provisioning.DefaultStatusPollInterval)🤖 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 `@cmd/main.go` around lines 406 - 432, The code currently constructs the AAP client and networking provider directly (aap.NewClient + provisioning.NewAAPProviderWithPrefix) which skips startup validation; instead call the existing createAAPProvider(...) helper to build and validate the shared networking provider (and obtain the validated aap client) and propagate any error to abort startup; then use the returned/validated aap client when creating publicIPAttachmentProvider (so replace direct aap.NewClient and provisioning.NewAAPProviderWithPrefix usages with the createAAPProvider call and error handling).
🤖 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 `@README.md`:
- Around line 30-33: The README overstates that "All controllers provision
infrastructure via direct Ansible Automation Platform REST API integration";
change this wording to clarify that only AAP-backed resource controllers require
direct AAP REST integration while tenant-only deployments and gRPC-only feedback
controllers do not; update the AAP provisioning section (the paragraph
mentioning controller behavior) to explicitly note that setupTenantController
can run without an AAP provider and that feedback controllers use gRPC-only, so
the document does not claim AAP is universally required.
---
Outside diff comments:
In `@cmd/main.go`:
- Around line 406-432: The code currently constructs the AAP client and
networking provider directly (aap.NewClient +
provisioning.NewAAPProviderWithPrefix) which skips startup validation; instead
call the existing createAAPProvider(...) helper to build and validate the shared
networking provider (and obtain the validated aap client) and propagate any
error to abort startup; then use the returned/validated aap client when creating
publicIPAttachmentProvider (so replace direct aap.NewClient and
provisioning.NewAAPProviderWithPrefix usages with the createAAPProvider call and
error handling).
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 10417f92-9cab-4727-8c79-b48687c8aee9
📒 Files selected for processing (44)
.claude/rules/configuration.mdAGENTS.mdREADME.mdapi/v1alpha1/job_types.gocharts/operator-crds/templates/osac.openshift.io_clusterorders.yamlcharts/operator-crds/templates/osac.openshift.io_computeinstances.yamlcharts/operator-crds/templates/osac.openshift.io_publicipattachments.yamlcharts/operator-crds/templates/osac.openshift.io_publicippools.yamlcharts/operator-crds/templates/osac.openshift.io_publicips.yamlcharts/operator-crds/templates/osac.openshift.io_securitygroups.yamlcharts/operator-crds/templates/osac.openshift.io_subnets.yamlcharts/operator-crds/templates/osac.openshift.io_tenants.yamlcharts/operator-crds/templates/osac.openshift.io_virtualnetworks.yamlcharts/operator/templates/deployment.yamlcharts/operator/values.yamlcmd/main.goconfig/crd/bases/osac.openshift.io_clusterorders.yamlconfig/crd/bases/osac.openshift.io_computeinstances.yamlconfig/crd/bases/osac.openshift.io_publicipattachments.yamlconfig/crd/bases/osac.openshift.io_publicippools.yamlconfig/crd/bases/osac.openshift.io_publicips.yamlconfig/crd/bases/osac.openshift.io_securitygroups.yamlconfig/crd/bases/osac.openshift.io_subnets.yamlconfig/crd/bases/osac.openshift.io_tenants.yamlconfig/crd/bases/osac.openshift.io_virtualnetworks.yamlconfig/manager/manager.yamlconfig/samples/osac-config-secret.yamlinternal/controller/clusterorder_controller_test.gointernal/controller/computeinstance_controller.gointernal/controller/computeinstance_controller_test.gointernal/controller/computeinstance_integration_test.gointernal/controller/computeinstance_provisioning_test.gointernal/controller/suite_test.gointernal/controller/webhook_common.gointernal/controller/webhook_common_test.gointernal/webhook/types.gopkg/provisioning/aap_provider.gopkg/provisioning/aap_provider_test.gopkg/provisioning/eda_provider.gopkg/provisioning/eda_provider_test.gopkg/provisioning/factory.gopkg/provisioning/factory_test.gopkg/provisioning/provider.gopkg/provisioning/provision_lifecycle.go
💤 Files with no reviewable changes (10)
- charts/operator/values.yaml
- config/manager/manager.yaml
- internal/controller/webhook_common.go
- pkg/provisioning/provision_lifecycle.go
- charts/operator/templates/deployment.yaml
- pkg/provisioning/eda_provider.go
- internal/webhook/types.go
- internal/controller/webhook_common_test.go
- internal/controller/computeinstance_controller.go
- pkg/provisioning/eda_provider_test.go
|
Re: cmd/main.go networking fail-fast (outside-diff comment from full review) — same answer as earlier. Deferring harmonized AAP credential validation to a follow-up outside OSAC-874 scope. The tenant controller’s optional AAP guard is intentional (storage provisioning no-op without creds; namespace/UDN lifecycle still runs). Cluster, compute, and networking controllers assume AAP is configured in production — installer overlays always set Also addressed the README wording in |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/retest-required |
akshaynadkarni
left a comment
akshaynadkarni
left a comment
There was a problem hiding this comment.
@tchughesiv Left a few nit comments. PTAL.
Thanks for working on this.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, jhernand, tchughesiv 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 |
|
/lgtm |
|
/lgtm |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/main.go (1)
208-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor: required AAP settings are not validated before these controllers start.
createAAPProvider()and the networking bootstrap both build an AAP client even whenOSAC_AAP_URLandOSAC_AAP_TOKENare empty. After this PR, cluster/compute/networking no longer have a non-AAP fallback, so the operator now boots successfully and only fails later on reconcile with opaque downstream errors.Suggested guard
+func validateAAPConfig(aapURL, aapToken string) error { + if aapURL == "" || aapToken == "" { + return fmt.Errorf("%s and %s must be set", envAAPURL, envAAPToken) + } + return nil +} + func createAAPProvider( aapURL, aapToken, provisionTemplate, deprovisionTemplate, templatePrefix string, aapInsecureSkipVerify bool, ) (provisioning.ProvisioningProvider, time.Duration, error) { + if err := validateAAPConfig(aapURL, aapToken); err != nil { + return nil, 0, err + } + statusPollInterval := helpers.GetEnvWithDefault(envAAPStatusPollInterval, provisioning.DefaultStatusPollInterval)aapURL := os.Getenv(envAAPURL) aapToken := os.Getenv(envAAPToken) + if err := validateAAPConfig(aapURL, aapToken); err != nil { + return err + } aapInsecureSkipVerify := helpers.GetEnvWithDefault(envAAPInsecureSkipVerify, false)Also applies to: 406-417
🤖 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 `@cmd/main.go` around lines 208 - 236, createAAPProvider currently constructs an AAP client even when required settings are empty; before calling aap.NewClient (and similarly before the networking bootstrap that also constructs an AAP client), validate the required AAP configuration (e.g., OSAC_AAP_URL and OSAC_AAP_TOKEN / the aapURL and aapToken parameters) and return a clear error if they are missing/empty instead of proceeding; update createAAPProvider (and the analogous bootstrap code that calls aap.NewClient) to check these values up-front and fail fast with a descriptive error so controllers do not start with an invalid AAP client.pkg/provisioning/aap_provider.go (1)
193-223:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMinor: HTTP 405 from
CancelJobis currently treated as “wait”, not “already terminal”.
cancelProvisionJob()converts*aap.MethodNotAllowedErrortonil, so the branch at Line 194-Line 200 can never fire. If the provision job flips terminal between the status GET and cancel POST, deletion is delayed by an unnecessary extra poll interval instead of proceeding immediately.Suggested fix
-func (p *AAPProvider) cancelProvisionJob(ctx context.Context, jobID string) error { +func (p *AAPProvider) cancelProvisionJob(ctx context.Context, jobID string) (bool, error) { err := p.client.CancelJob(ctx, jobID) if err != nil { var methodNotAllowedErr *aap.MethodNotAllowedError if errors.As(err, &methodNotAllowedErr) { - return nil + return true, nil } - return fmt.Errorf("failed to cancel job: %w", err) + return false, fmt.Errorf("failed to cancel job: %w", err) } - - return nil + return false, nil }- if err := p.cancelProvisionJob(ctx, latestProvisionJob.JobID); err != nil { + alreadyTerminal, err := p.cancelProvisionJob(ctx, latestProvisionJob.JobID) + if err != nil { var methodNotAllowedErr *aap.MethodNotAllowedError if !errors.As(err, &methodNotAllowedErr) { return false, &status, fmt.Errorf("failed to cancel provision job: %w", err) } - // 405 means already terminal, proceed - log.Info("job cancel returned 405 (already terminal), ready to deprovision", "jobID", latestProvisionJob.JobID) - return true, &status, nil } + if alreadyTerminal { + log.Info("job cancel returned 405 (already terminal), ready to deprovision", "jobID", latestProvisionJob.JobID) + return true, &status, nil + }🤖 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 `@pkg/provisioning/aap_provider.go` around lines 193 - 223, The cancelProvisionJob function currently swallows *aap.MethodNotAllowedError by returning nil, preventing the caller (the provision cancellation branch that checks errors.As(err, &methodNotAllowedErr)) from detecting a 405 and proceeding immediately; change cancelProvisionJob (in AAPProvider) to return the original *aap.MethodNotAllowedError (or wrap and return it) instead of nil when errors.As detects it so the caller's errors.As(check) on the result of cancelProvisionJob(ctx, latestProvisionJob.JobID) can detect the 405 and take the "already terminal" 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 `@pkg/provisioning/aap_provider_test.go`:
- Around line 555-589: Add a parallel test Context that simulates the AAP
cancelJob call returning an HTTP 405 (job already terminal) by setting
aapClient.cancelJobFunc to return an error that encodes status 405, then call
provider.TriggerDeprovision(ctx, instance) and assert the code path for the 405
case: expect the provider to treat the job as terminal and proceed to the
immediate-delete branch (i.e., expect result.Action to be
provisioning.DeprovisionImmediate and that ProvisionJobStatus reflects no
running job), using the same symbols used in the existing test
(provider.TriggerDeprovision and aapClient.cancelJobFunc).
---
Outside diff comments:
In `@cmd/main.go`:
- Around line 208-236: createAAPProvider currently constructs an AAP client even
when required settings are empty; before calling aap.NewClient (and similarly
before the networking bootstrap that also constructs an AAP client), validate
the required AAP configuration (e.g., OSAC_AAP_URL and OSAC_AAP_TOKEN / the
aapURL and aapToken parameters) and return a clear error if they are
missing/empty instead of proceeding; update createAAPProvider (and the analogous
bootstrap code that calls aap.NewClient) to check these values up-front and fail
fast with a descriptive error so controllers do not start with an invalid AAP
client.
In `@pkg/provisioning/aap_provider.go`:
- Around line 193-223: The cancelProvisionJob function currently swallows
*aap.MethodNotAllowedError by returning nil, preventing the caller (the
provision cancellation branch that checks errors.As(err, &methodNotAllowedErr))
from detecting a 405 and proceeding immediately; change cancelProvisionJob (in
AAPProvider) to return the original *aap.MethodNotAllowedError (or wrap and
return it) instead of nil when errors.As detects it so the caller's
errors.As(check) on the result of cancelProvisionJob(ctx,
latestProvisionJob.JobID) can detect the 405 and take the "already terminal"
path.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f4e4d902-8bdf-4ff8-9a17-5b70ffe26ebf
📒 Files selected for processing (45)
.claude/rules/configuration.md.claude/rules/controller-patterns.mdAGENTS.mdREADME.mdapi/v1alpha1/job_types.gocharts/operator-crds/templates/osac.openshift.io_clusterorders.yamlcharts/operator-crds/templates/osac.openshift.io_computeinstances.yamlcharts/operator-crds/templates/osac.openshift.io_publicipattachments.yamlcharts/operator-crds/templates/osac.openshift.io_publicippools.yamlcharts/operator-crds/templates/osac.openshift.io_publicips.yamlcharts/operator-crds/templates/osac.openshift.io_securitygroups.yamlcharts/operator-crds/templates/osac.openshift.io_subnets.yamlcharts/operator-crds/templates/osac.openshift.io_tenants.yamlcharts/operator-crds/templates/osac.openshift.io_virtualnetworks.yamlcharts/operator/templates/deployment.yamlcharts/operator/values.yamlcmd/main.goconfig/crd/bases/osac.openshift.io_clusterorders.yamlconfig/crd/bases/osac.openshift.io_computeinstances.yamlconfig/crd/bases/osac.openshift.io_publicipattachments.yamlconfig/crd/bases/osac.openshift.io_publicippools.yamlconfig/crd/bases/osac.openshift.io_publicips.yamlconfig/crd/bases/osac.openshift.io_securitygroups.yamlconfig/crd/bases/osac.openshift.io_subnets.yamlconfig/crd/bases/osac.openshift.io_tenants.yamlconfig/crd/bases/osac.openshift.io_virtualnetworks.yamlconfig/manager/manager.yamlconfig/samples/osac-config-secret.yamlinternal/controller/clusterorder_controller_test.gointernal/controller/computeinstance_controller.gointernal/controller/computeinstance_controller_test.gointernal/controller/computeinstance_integration_test.gointernal/controller/computeinstance_provisioning_test.gointernal/controller/suite_test.gointernal/controller/webhook_common.gointernal/controller/webhook_common_test.gointernal/webhook/types.gopkg/provisioning/aap_provider.gopkg/provisioning/aap_provider_test.gopkg/provisioning/eda_provider.gopkg/provisioning/eda_provider_test.gopkg/provisioning/factory.gopkg/provisioning/factory_test.gopkg/provisioning/provider.gopkg/provisioning/provision_lifecycle.go
💤 Files with no reviewable changes (9)
- charts/operator/templates/deployment.yaml
- internal/controller/webhook_common.go
- internal/controller/webhook_common_test.go
- internal/webhook/types.go
- config/manager/manager.yaml
- pkg/provisioning/eda_provider_test.go
- pkg/provisioning/eda_provider.go
- pkg/provisioning/provision_lifecycle.go
- charts/operator/values.yaml
|
Re: latest CodeRabbit review (outside-diff items):
|
|
New changes are detected. LGTM label has been removed. |
Add blank lines after headings to satisfy MD022.
Propagate MethodNotAllowedError from cancelProvisionJob so deprovision proceeds immediately when cancel returns 405. Add matching unit test.
b2404f8 to
9f02f51
Compare
| // Job is already in terminal state, nothing to cancel | ||
| return nil | ||
| // Propagate 405 so the caller can proceed immediately instead of waiting another poll. | ||
| return err |
There was a problem hiding this comment.
@tchughesiv Returning 405 here seems logical, especially since the caller is explicity checking for it:
https://github.com/tchughesiv/osac-operator/blame/9f02f51fc54da2d6248c0dca78bcc01ae14b922d/pkg/provisioning/aap_provider.go#L192-L199
Please update the comment a few lines above: Line 209, Line 214, since we are no longer returning nil.
I'm not sure why "405: Method Not Allowed" was equating to "job is already in terminal state". @tzvatot What was your thought process here?
6 participants
Summary
Removes the legacy EDA webhook provisioning provider from osac-operator. The operator now always uses AAP direct REST API integration. Provider selection configuration is deleted entirely (
OSAC_PROVISIONING_PROVIDER, webhook URLs,OSAC_MINIMUM_REQUEST_INTERVAL).Changes
eda_provider,webhook_common, andinternal/webhookpackages (~990 lines)aap_providermain(includes fix: Add missing config/testing/console-proxy kustomize overlay #276 test-kustomize overlay)Related PRs
Jira
https://redhat.atlassian.net/browse/OSAC-874
Validation (2026-06-02, HEAD
be521ed)origin/main— 0 commits behindmake fmt,make lint,make build,make testmake manifests generate— no driftmake check-helm-crdsmake test-kustomize— pass (includesconfig/testing/console-proxyvia fix: Add missing config/testing/console-proxy kustomize overlay #276)make test-smokeAcceptance criteria
aap)test-kustomize,check-helm-crds); smoke deferredSummary by CodeRabbit
Documentation
Refactor