Skip to content

WIP: Fake bump library-go to test#2162

Closed
ardaguclu wants to merge 3 commits into
openshift:mainfrom
ardaguclu:fake-bump
Closed

WIP: Fake bump library-go to test#2162
ardaguclu wants to merge 3 commits into
openshift:mainfrom
ardaguclu:fake-bump

Conversation

@ardaguclu
Copy link
Copy Markdown
Member

@ardaguclu ardaguclu commented May 21, 2026

Summary by CodeRabbit

  • Chores

    • Updated module dependency configuration.
  • Tests

    • Updated encryption end-to-end and performance tests to use context-aware test helpers and the default fake KMS provider wiring.

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 21, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9cdc2d3c-1c8a-4b67-badb-108a48679d76

📥 Commits

Reviewing files that changed from the base of the PR and between 52d1c0c and e931d2b.

⛔ Files ignored due to path filters (12)
  • go.sum is excluded by !**/*.sum
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/config.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/secret.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/helpers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/secrets.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/state/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/helpers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/kms/vault.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/perf_scenarios.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/scenarios.go is excluded by !**/vendor/**, !vendor/**
  • vendor/modules.txt is excluded by !**/vendor/**, !vendor/**
📒 Files selected for processing (6)
  • go.mod
  • test/e2e-encryption-kms/encryption_kms.go
  • test/e2e-encryption-perf/encryption_perf_test.go
  • test/e2e-encryption-rotation/encryption_rotation_test.go
  • test/e2e-encryption/encryption_test.go
  • test/e2e/encryption.go

Walkthrough

The PR reformats the ginkgo v2 replace into a grouped replace (...) and adds a library-go replace to a fork; it also updates multiple e2e encryption tests to import context and pass context.TODO() into test helper functions, and replaces manual KMS provider construction with librarykms.DefaultFakeVaultEncryptionProvider.

Changes

Module Replacement Updates & e2e encryption tests

Layer / File(s) Summary
Go module replace directives
go.mod
The ginkgo v2 replacement is reformatted into a multi-entry replace (...) block, and a new replacement for github.com/openshift/library-go pointing to github.com/ardaguclu/library-go is added.
KMS e2e test wiring and provider change
test/e2e-encryption-kms/encryption_kms.go
Removed unused configv1 import, pass context.TODO() into helper calls, and replace manual library.EncryptionProvider KMS entries with librarykms.DefaultFakeVaultEncryptionProvider.
Other e2e encryption tests: add context to helper calls
test/e2e-encryption-perf/encryption_perf_test.go, test/e2e-encryption-rotation/encryption_rotation_test.go, test/e2e/encryption.go, test/e2e-encryption/encryption_test.go
Import context where needed and pass context.TODO() as the first argument to various library. test helper functions to match updated context-aware signatures.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • benluddy
🚥 Pre-merge checks | ✅ 12
✅ Passed checks (12 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main changes: redirecting library-go dependency to a fake version and updating test invocations to pass context arguments.
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.
Stable And Deterministic Test Names ✅ Passed All Ginkgo test names in modified files are static, deterministic strings without dynamic values like pod names, UUIDs, timestamps, node names, or namespace suffixes.
Test Structure And Quality ✅ Passed Modified tests follow codebase patterns with timeouts and assertion messages. Tests are thin wrappers delegating to library functions, consistent with encryption test architecture.
Microshift Test Compatibility ✅ Passed No new Ginkgo e2e tests added; PR only updates existing test invocations. Existing tests have MicroShift compatibility protections.
Single Node Openshift (Sno) Test Compatibility ✅ Passed PR contains no new Ginkgo e2e test declarations, only modifications to existing test function calls to add context arguments. SNO compatibility check applies only to new tests.
Topology-Aware Scheduling Compatibility ✅ Passed PR modifies only test files and dependency management (go.mod). No deployment manifests, operator code, or scheduling constraints are introduced.
Ote Binary Stdout Contract ✅ Passed PR only modifies test case code (inside It/TestXxx functions) adding context parameters. No stdout writes in process-level code detected.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed New Ginkgo e2e tests added contain no IPv4 assumptions, hardcoded IPs, or external connectivity requirements. Tests use only cluster-internal resources and delegate to library functions.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 21, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ricardomaraschini for approval. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@go.mod`:
- Line 140: The go.mod currently replaces github.com/openshift/library-go with a
personal fork github.com/ardaguclu/library-go v0.0.0-20260521063842-0a19ee496ade
which weakens supply-chain trust; remove or revert the replace entry that points
github.com/openshift/library-go to github.com/ardaguclu/library-go (the replace
directive shown in the diff) and restore the canonical upstream module, or if
this fork is absolutely required obtain documented approval and move the
exception out of committed go.mod (e.g., use a local/private replace in CI-only
scripts or documented override) so shared CI/release uses the upstream module.
🪄 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: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 133225b2-8724-4868-977d-879d1e729b6d

📥 Commits

Reviewing files that changed from the base of the PR and between 32aa38e and 52d1c0c.

⛔ Files ignored due to path filters (8)
  • go.sum is excluded by !**/*.sum
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/config.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptiondata/secret.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/secrets.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/state/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/modules.txt is excluded by !**/vendor/**, !vendor/**
📒 Files selected for processing (1)
  • go.mod

Comment thread go.mod Outdated
replace github.com/onsi/ginkgo/v2 => github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20251001123353-fd5b1fb35db1
replace (
github.com/onsi/ginkgo/v2 => github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20251001123353-fd5b1fb35db1
github.com/openshift/library-go => github.com/ardaguclu/library-go v0.0.0-20260521063842-0a19ee496ade
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read-only verification: confirm the replacement currently points to a personal fork.
rg -n '^\s*replace\s*\(|github.com/openshift/library-go|github.com/ardaguclu/library-go' go.mod

Repository: openshift/cluster-kube-apiserver-operator

Length of output: 277


Block personal-fork replace for library-go in go.mod
go.mod line 140 replaces github.com/openshift/library-go with the personal fork github.com/ardaguclu/library-go v0.0.0-20260521063842-0a19ee496ade, weakening supply-chain trust and reproducibility for shared CI/release. Revert to the canonical upstream module before merge, or add a clearly documented and approved exception outside committed go.mod.

🤖 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 `@go.mod` at line 140, The go.mod currently replaces
github.com/openshift/library-go with a personal fork
github.com/ardaguclu/library-go v0.0.0-20260521063842-0a19ee496ade which weakens
supply-chain trust; remove or revert the replace entry that points
github.com/openshift/library-go to github.com/ardaguclu/library-go (the replace
directive shown in the diff) and restore the canonical upstream module, or if
this fork is absolutely required obtain documented approval and move the
exception out of committed go.mod (e.g., use a local/private replace in CI-only
scripts or documented override) so shared CI/release uses the upstream module.

@tjungblu
Copy link
Copy Markdown
Contributor

/uncc

@openshift-ci openshift-ci Bot removed the request for review from tjungblu May 21, 2026 07:12
func TestPerfEncryption(tt *testing.T) {
operatorClient := operatorencryption.GetOperator(tt)
library.TestPerfEncryption(tt, library.PerfScenario{
library.TestPerfEncryption(context.TODO(), tt, library.PerfScenario{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use library.TestPerfEncryption(tt.Context(), tt, library.PerfScenario{?

// file
func TestEncryptionRotation(t *testing.T) {
library.TestEncryptionRotation(t, library.RotationScenario{
library.TestEncryptionRotation(context.TODO(), t, library.RotationScenario{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t.context()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same all places


func TestEncryptionTypeIdentity(t *testing.T) {
library.TestEncryptionTypeIdentity(t, library.BasicScenario{
library.TestEncryptionTypeIdentity(context.TODO(), t, library.BasicScenario{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t.context()?

Comment thread test/e2e/encryption.go

func testEncryptionTypeAESCBC(t testing.TB) {
library.TestEncryptionTypeAESCBC(t, library.BasicScenario{
library.TestEncryptionTypeAESCBC(context.TODO(), t, library.BasicScenario{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same above

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm planning to close this PR, once openshift/library-go#2212 merges. Thanks for review.


func TestEncryptionTurnOnAndOff(t *testing.T) {
library.TestEncryptionTurnOnAndOff(t, library.OnOffScenario{
library.TestEncryptionTurnOnAndOff(context.TODO(), t, library.OnOffScenario{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@ardaguclu
Copy link
Copy Markdown
Member Author

I'm closing this PR in favor of #2164
/close

@openshift-ci openshift-ci Bot closed this May 21, 2026
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 21, 2026

@ardaguclu: Closed this PR.

Details

In response to this:

I'm closing this PR in favor of #2164
/close

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 kubernetes-sigs/prow repository.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 21, 2026

@ardaguclu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-operator-encryption-kms e931d2b link false /test e2e-gcp-operator-encryption-kms
ci/prow/e2e-gcp-operator-encryption-single-node e931d2b link false /test e2e-gcp-operator-encryption-single-node
ci/prow/e2e-gcp-operator-encryption-perf-aescbc e931d2b link false /test e2e-gcp-operator-encryption-perf-aescbc
ci/prow/e2e-gcp-operator-encryption-perf-single-node e931d2b link false /test e2e-gcp-operator-encryption-perf-single-node

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@ardaguclu ardaguclu deleted the fake-bump branch May 21, 2026 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants