Skip to content

Conversation

leggetter
Copy link
Collaborator

No description provided.

@Copilot Copilot AI review requested due to automatic review settings October 9, 2025 15:10
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for Google Cloud Platform (GCP) Pub/Sub as a destination type, including comprehensive configuration and credentials schemas for the OpenAPI specification.

  • Defines GCP Pub/Sub configuration schema with project ID, topic name, and optional endpoint
  • Adds service account JSON credentials schema for authentication
  • Integrates GCP Pub/Sub into polymorphic destination schemas for create, read, and update operations

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

DestinationGCPPubSub:
type: object
# Properties duplicated from DestinationBase
required: [id, type, topics, config, credentials, created_at, disabled_at]
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

The 'disabled_at' field should not be required since it can be null when the destination is enabled. Consider removing it from the required array or making it optional.

Suggested change
required: [id, type, topics, config, credentials, created_at, disabled_at]
required: [id, type, topics, config, credentials, created_at]

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is consistent with other destination types

@vercel
Copy link

vercel bot commented Oct 9, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
outpost-docs Ready Ready Preview Comment Oct 13, 2025 0:32am
outpost-website Ready Ready Preview Comment Oct 13, 2025 0:32am

@leggetter leggetter changed the title feat: add GCP Pub/Sub destination support with configuration and credntials schemas feat: add GCP Pub/Sub destination support with configuration and credentials schemas Oct 9, 2025
…tract

This is a WIP and only tests a subset of the functionality.
…e Bus, Hookdeck, RabbitMQ, and Webhook

- Implemented contract tests for Azure Service Bus destinations, covering creation, retrieval, listing, updating, and deletion.
- Added contract tests for Hookdeck destinations with similar coverage.
- Created contract tests for RabbitMQ destinations, ensuring validation of required fields and error handling.
- Developed contract tests for Webhook destinations, including validation for required configuration fields.
- Each test suite includes setup and teardown logic to manage test tenant and destination cleanup.
… OpenAPI validation test suite

- Create contributing documentation outlining test suite architecture, guidelines for adding tests, and best practices.
- Establish implementation order and roadmap for enhancing the test suite with CI/CD integration, coverage reporting, and documentation.
- Update README with current state of the test suite and next phases for development.
Add missing 'status' field to Event schema in OpenAPI specification
and generate updated TypeScript SDK with proper type definitions.

Changes:
- Updated Event schema in OpenAPI spec to include 'status' field
  (enum: success | failed)
- Regenerated TypeScript SDK with new EventStatus type
- Added comprehensive test suite for event status field validation
- Updated all destination tests to use proper SDK types
- Added SDK regeneration script for consistent build process

The API was already returning the status field in event responses,
but it was not documented in the OpenAPI spec, causing SDK clients
to not have access to this field. This change aligns the spec with
the actual API behavior.

Tests verify:
- Status field is present in events from listByDestination
- Status field is present in single event retrieval
- Status field is present in tenant-wide event lists
- Status values are valid (success or failed)

Fixes #490
Related to PR #491
@leggetter
Copy link
Collaborator Author

@claude - can you review the PR?

@claude
Copy link

claude bot commented Oct 13, 2025

Claude finished @leggetter's task —— View job


Code Review Complete ✓

Reviewing PR #518: GCP Pub/Sub destination support

Review Checklist:

  • Read and analyze OpenAPI schema changes
  • Compare GCP Pub/Sub implementation with other destination types for consistency
  • Review credential handling and security considerations
  • Examine test infrastructure additions
  • Review example code changes
  • Provide comprehensive feedback

Summary

This PR adds comprehensive GCP Pub/Sub destination support to Outpost, including OpenAPI schema definitions, SDK generation, test coverage, and backend implementation. The implementation is well-structured and follows the existing patterns for other destination types.

Positive Aspects ✅

  1. Consistent Schema Design: The GCP Pub/Sub schemas (GCPPubSubConfig, GCPPubSubCredentials, DestinationGCPPubSub, etc.) follow the exact same structure as other destination types (AWS SQS, Azure Service Bus, etc.), ensuring consistency across the API.

  2. Complete Integration: The destination type is properly integrated into all polymorphic schemas:

    • Destination (read/response)
    • DestinationCreate (create request)
    • DestinationUpdate (update request)
    • All discriminator mappings are correct
  3. Comprehensive Test Coverage: The test file spec-sdk-tests/tests/destinations/gcp-pubsub.test.ts is thorough and includes:

    • CRUD operations (Create, Read, Update, Delete)
    • Edge cases (long topic names, special characters)
    • Error handling (missing required fields, non-existent resources)
    • Optional configuration (endpoint for emulator support)
  4. Security Considerations: Service account JSON credentials are properly marked as required and the schema clearly indicates they should be stored securely (consistent with other credential types).

  5. Backend Implementation Alignment: The backend implementation in destgcppubsub.go correctly maps to the OpenAPI schema:

    • Config fields: project_id, topic, endpoint (optional)
    • Credentials: service_account_json
    • Proper target URL construction for GCP Console
  6. Documentation: Good inline documentation with clear descriptions and examples for all fields.

Issues and Recommendations 🔍

1. Critical: Inconsistent disabled_at Required Field

Location: docs/apis/openapi.yaml:697

The disabled_at field is marked as required in the DestinationGCPPubSub schema, but it can be null when the destination is enabled. This is consistent with other destination types, but as the copilot reviewer noted, having a nullable field in the required array is semantically confusing.

Current:

required: [id, type, topics, config, credentials, created_at, disabled_at]

Recommendation: While you've indicated this is consistent with other destination types (which it is), consider whether this pattern should be reviewed across all destination types in a separate PR. The OpenAPI 3.1 spec allows nullable types, but marking a nullable field as required means "the field must be present, but its value can be null." This is technically correct but could be clearer with better documentation.

Action: ✅ No change needed for this PR since it maintains consistency. Consider documenting this pattern or addressing it system-wide in a future PR.

2. Minor: Schema Validation Edge Cases

Location: Test file shows several skipped tests

The test file has TODO comments for tests that are skipped:

  • Line 84-85: created_at property check is disabled
  • Line 218: Invalid serviceAccountJson validation is skipped
  • Line 516: Invalid config on update validation is skipped

Recommendation: While these are clearly marked as TODO items for future backend improvements, consider adding GitHub issues to track these validation gaps if they don't already exist.

3. Documentation: Service Account JSON Format

Location: docs/apis/openapi.yaml:307-310

The example for service_account_json is good, but it might be helpful to add a note about the complete required fields for a valid GCP service account JSON.

Current:

service_account_json:
  type: string
  description: Service account key JSON. The entire JSON key file content as a string.
  example: '{"type":"service_account","project_id":"my-project","private_key_id":"key123",...}'

Recommendation: Consider adding a note that this should be the complete JSON downloaded from GCP Console, including all required fields like private_key, client_email, etc.

4. Minor: Type Enum Consistency

Location: Multiple locations in OpenAPI spec

The destination type enums in query parameters (lines 1469-1478, 1483-1492, 1991, 2538) need to include gcp_pubsub everywhere. I can see gcp_pubsub is already included at lines 1477 and 1491, which is correct. ✅

5. Test: Missing Integration with Backend

Location: spec-sdk-tests/tests/destinations/gcp-pubsub.test.ts

The tests create destinations with test credentials that won't actually connect to GCP. While this is appropriate for contract testing, consider adding a note in the test file about whether there are separate integration tests that actually publish to a real GCP Pub/Sub emulator or test project.

Security Review 🔒

  1. ✅ Credentials are properly segregated in the credentials field
  2. ✅ Service account JSON is stored as a string (encrypted at rest per the schema comments)
  3. ✅ No credentials are logged or exposed in error messages
  4. ✅ Optional endpoint field allows for emulator testing without exposing production credentials

Generated SDK Quality

The generated TypeScript SDK files show:

  • ✅ Proper Zod validation schemas
  • ✅ Correct camelCase to snake_case conversion
  • ✅ Type-safe interfaces
  • ✅ JSON serialization/deserialization helpers

Overall Assessment

This is a high-quality PR that follows best practices and maintains consistency with the existing codebase. The implementation is thorough, well-tested, and properly documented.

Recommendation: Approve with minor suggestions

The PR is ready to merge. The only "issue" raised by the copilot reviewer about disabled_at being required but nullable is actually consistent with the existing architecture and is acceptable. The skipped tests are clearly marked as TODOs for future backend improvements.

Optional Follow-up Items (Not Blockers):

  1. Create issues for the skipped tests to track backend validation improvements
  2. Consider adding integration tests that use the GCP Pub/Sub emulator
  3. Document the nullable-but-required pattern for timestamp fields across all destination types
  4. Add a note in the schema about the complete service account JSON format requirements

Files Reviewed:

  • docs/apis/openapi.yaml (main schema changes)
  • sdks/outpost-typescript/src/models/components/gcppubsubconfig.ts
  • sdks/outpost-typescript/src/models/components/gcppubsubcredentials.ts
  • internal/destregistry/providers/destgcppubsub/destgcppubsub.go
  • spec-sdk-tests/tests/destinations/gcp-pubsub.test.ts
  • spec-sdk-tests/factories/destination.factory.ts

Branch: chore/openapi-add-gcppubsub

@leggetter leggetter merged commit a62b2a7 into main Oct 13, 2025
4 checks passed
@leggetter leggetter deleted the chore/openapi-add-gcppubsub branch October 13, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants