SANDBOX-1889: update PhoneLookupMode default fallback to disabled

[fbm3307]

Change the runtime nil fallback in PhoneLookupMode() from PhoneLookupModeLog to PhoneLookupModeDisabled, and update the corresponding test assertion.

Related Prs
API: codeready-toolchain/api#515
host-operator: codeready-toolchain/host-operator#1274

Toolchain-e2e: codeready-toolchain/toolchain-e2e#1292

Assisted By : Cursor

Summary by CodeRabbit

• Bug Fixes
  • Updated the default phone lookup setting to be disabled when no value is configured.
  • Aligned the expected default behavior in tests with the new configuration default.

[coderabbitai]

Walkthrough

The default return value of VerificationConfig.PhoneLookupMode() when the underlying configuration value is nil was changed from PhoneLookupModeLog to PhoneLookupModeDisabled. The corresponding test assertion was updated to match this new default.

Changes

PhoneLookupMode default behavior

Layer / File(s)	Summary
Default value change and test update pkg/configuration/configuration.go, pkg/configuration/configuration_test.go	PhoneLookupMode() now defaults to PhoneLookupModeDisabled instead of PhoneLookupModeLog when nil, and the test assertion is updated accordingly.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Suggested labels: bug, test

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: the PhoneLookupMode nil fallback now defaults to disabled.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/configuration/configuration.go (1)

332-337: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Keep the phone lookup default aligned across repos. Changing the nil fallback to PhoneLookupModeDisabled now disagrees with the api type comments/OpenAPI/CRD, and toolchain-e2e still treats an unset value as PhoneLookupModeLog. Update the generated docs/CRD and the e2e wait helper in lockstep, or the default will be documented and tested inconsistently.

🤖 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/configuration/configuration.go` around lines 332 - 337, The
PhoneLookupMode default in VerificationConfig.PhoneLookupMode is now
inconsistent with the API docs and toolchain-e2e expectations. Update the nil
fallback and keep it aligned with the generated OpenAPI/CRD comments plus the
e2e wait helper that currently assumes an unset value maps to
PhoneLookupModeLog, so all repos use the same default behavior. Locate the
related logic in VerificationConfig.PhoneLookupMode and the corresponding api
type definitions/generated docs and toolchain-e2e helper, then change them
together.

Source: Linked repositories

🤖 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.

Outside diff comments:
In `@pkg/configuration/configuration.go`:
- Around line 332-337: The PhoneLookupMode default in
VerificationConfig.PhoneLookupMode is now inconsistent with the API docs and
toolchain-e2e expectations. Update the nil fallback and keep it aligned with the
generated OpenAPI/CRD comments plus the e2e wait helper that currently assumes
an unset value maps to PhoneLookupModeLog, so all repos use the same default
behavior. Locate the related logic in VerificationConfig.PhoneLookupMode and the
corresponding api type definitions/generated docs and toolchain-e2e helper, then
change them together.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: de13c309-fd2a-433e-a92d-89978f997541

📥 Commits

Reviewing files that changed from the base of the PR and between e8ab074 and 7f032c8.

📒 Files selected for processing (2)

• pkg/configuration/configuration.go
• pkg/configuration/configuration_test.go

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• codeready-toolchain/api (manual)
• codeready-toolchain/toolchain-common (manual)
• codeready-toolchain/host-operator (manual)
• codeready-toolchain/toolchain-e2e (manual)

📜 Review details

⏰ Context from checks skipped due to timeout. (4)

• GitHub Check: GolangCI Lint
• GitHub Check: test
• GitHub Check: govulncheck
• GitHub Check: Build & push operator bundles & dashboard image for e2e tests

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/configuration/configuration.go
• pkg/configuration/configuration_test.go

🔀 Multi-repo context

Linked repositories findings

codeready-toolchain/api

• api/v1alpha1/toolchainconfig_types.go:400-405 still documents PhoneLookupMode as defaulting to "log" in the API type comments.
• api/v1alpha1/zz_generated.openapi.go:2950 and api/v1alpha1/docs/apiref.adoc:2340 also describe the default as "log".
• This is relevant because the PR changes the runtime nil fallback to disabled, so API/docs now disagree with behavior.

codeready-toolchain/host-operator

• config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml:361-370 still has phoneLookupMode default log with description saying it defaults to "log".
• This CRD default may now conflict with the new nil fallback if consumers rely on the generated schema/defaulting behavior.

codeready-toolchain/toolchain-e2e

• testsupport/wait/host.go:1728-1740 hardcodes mode := toolchainv1alpha1.PhoneLookupModeLog when PhoneLookupMode is nil.
• That helper encodes the old assumption that nil means log, so it may now misreport/expect the wrong value in tests that leave the field unset.
• test/e2e/parallel/phone_lookup_test.go:40 and nearby tests explicitly set PhoneLookupMode(Log|Enabled), so those cases are unaffected.

codeready-toolchain/toolchain-common

• pkg/test/config/toolchainconfig.go:508-517 only sets explicit PhoneLookupMode values in test config builders.
• No direct breakage found here from the nil-fallback change.

🔇 Additional comments (1)

pkg/configuration/configuration_test.go (1)

72-72: LGTM!

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, fbm3307, MatousJobanek, metlos

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [MatousJobanek,alexeykazakov,fbm3307,metlos]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud