Skip to content
This repository was archived by the owner on Sep 9, 2026. It is now read-only.

OSAC-830: Add private server for Public IP Attachments resource - #542

Merged
openshift-merge-bot[bot] merged 1 commit into
osac-project:mainfrom
DakCrowder:private-public-ip-attachment-server
May 18, 2026
Merged

openshift-merge-bot[bot] merged 1 commit into
osac-project:mainfrom
DakCrowder:private-public-ip-attachment-server

Conversation

@DakCrowder

@DakCrowder DakCrowder commented May 15, 2026 •

Copy link
Copy Markdown
Contributor

OSAC-830

Implements the private server for Public IP Attachment resources. There is currently no validation and only basic CRUD is implemented - follow up tickets are planned for validation and the public server.

@openshift-ci-robot

openshift-ci-robot commented May 15, 2026 •

Copy link
Copy Markdown

@DakCrowder: This pull request references OSAC-830 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 sub-task to target the "5.0.0" version, but no target version was set.

Details

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

@openshift-ci
openshift-ci Bot requested review from akshaynadkarni and jhernand May 15, 2026 14:12
@DakCrowder
DakCrowder requested a review from SiddarthR56 May 15, 2026 14:20
@coderabbitai

coderabbitai Bot commented May 15, 2026 •

Copy link
Copy Markdown

Review Change Stack

Summary by CodeRabbit

  • New Features

    • Introduced private API for public IP attachments with full CRUD, signal, and REST/gRPC exposure.
  • Tests

    • Added comprehensive test suite validating server builder configuration and all operations (create, list, get, update, delete, signal).
  • Chores

    • Added database migrations to create and rollback tables and indexes for public IP attachments.

Walkthrough

This pull request introduces a new PublicIPAttachments resource to the fulfillment service. The implementation spans database schema definition via migrations, a private gRPC server with fluent builder dependency injection, integration into both gRPC and REST gateway startup flows, and comprehensive test coverage. The database layer defines public_ip_attachments and archived_public_ip_attachments tables with JSONB and array columns plus optimized indexes. The server delegates CRUD and signal operations to an underlying generic server instance configured with authentication, tenancy, and observability dependencies.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: adding a private server implementation for the Public IP Attachments resource, which aligns with all file changes in the PR.
Description check ✅ Passed The description accurately explains the changeset, referencing the Jira issue (OSAC-830) and noting that it implements a private server for Public IP Attachment resources with basic CRUD and planned follow-up work.
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.

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


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.

❤️ Share

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

@DakCrowder

Copy link
Copy Markdown
Contributor Author

/test e2e-vmaas

@DakCrowder

Copy link
Copy Markdown
Contributor Author

/retest

@DakCrowder
DakCrowder force-pushed the private-public-ip-attachment-server branch from bbe1357 to 7ddda1a Compare May 18, 2026 13:26
@openshift-ci openshift-ci Bot removed the lgtm label May 18, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
internal/database/migrations/38_create_public_ip_attachments_tables.up.sql (1)

20-20: ⚡ Quick win

Consider adding an index on deletion_timestamp for improved query performance.

Queries filtering active versus soft-deleted records (e.g., WHERE deletion_timestamp = 'epoch') are a common pattern. Without an index on deletion_timestamp, these queries require full table scans as the table grows.

📊 Suggested index addition
 create index public_ip_attachments_by_name on public_ip_attachments (name);
+create index public_ip_attachments_by_deletion_timestamp on public_ip_attachments (deletion_timestamp);
 create index public_ip_attachments_by_owner on public_ip_attachments using gin (creators);
🤖 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 `@internal/database/migrations/38_create_public_ip_attachments_tables.up.sql`
at line 20, Add an index on the deletion_timestamp column in the migration
(internal/database/migrations/38_create_public_ip_attachments_tables.up.sql) to
speed up common soft-delete filters; update the migration that defines the
deletion_timestamp column to create an index (or a partial index targeting the
common predicate like WHERE deletion_timestamp = 'epoch' or WHERE
deletion_timestamp <> 'epoch') so queries filtering active vs soft-deleted
records use the index instead of full table scans.
🤖 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.

Nitpick comments:
In `@internal/database/migrations/38_create_public_ip_attachments_tables.up.sql`:
- Line 20: Add an index on the deletion_timestamp column in the migration
(internal/database/migrations/38_create_public_ip_attachments_tables.up.sql) to
speed up common soft-delete filters; update the migration that defines the
deletion_timestamp column to create an index (or a partial index targeting the
common predicate like WHERE deletion_timestamp = 'epoch' or WHERE
deletion_timestamp <> 'epoch') so queries filtering active vs soft-deleted
records use the index instead of full table scans.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 309a8995-2157-4b70-b613-b1c12559f239

📥 Commits

Reviewing files that changed from the base of the PR and between bbe1357 and 7ddda1a.

📒 Files selected for processing (6)
  • internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go
  • internal/cmd/service/start/restgateway/start_rest_gateway_cmd.go
  • internal/database/migrations/38_create_public_ip_attachments_tables.down.sql
  • internal/database/migrations/38_create_public_ip_attachments_tables.up.sql
  • internal/servers/private_public_ip_attachments_server.go
  • internal/servers/private_public_ip_attachments_server_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
  • internal/database/migrations/38_create_public_ip_attachments_tables.down.sql
  • internal/cmd/service/start/restgateway/start_rest_gateway_cmd.go
  • internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go
  • internal/servers/private_public_ip_attachments_server.go
  • internal/servers/private_public_ip_attachments_server_test.go

@openshift-ci openshift-ci Bot added the lgtm label May 18, 2026
@openshift-ci

openshift-ci Bot commented May 18, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DakCrowder, jhernand

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:

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants