Skip to content

Conversation

@ronething
Copy link
Contributor

@ronething ronething commented May 14, 2025

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches
  • Documentation
  • Refactor
  • Chore
  • CI/CD or Tests

What this PR does / why we need it:

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@github-actions
Copy link
Contributor

github-actions bot commented May 14, 2025

conformance test report

apiVersion: gateway.networking.k8s.io/v1
date: "2025-05-14T09:59:43Z"
gatewayAPIChannel: standard
gatewayAPIVersion: v1.2.0
implementation:
  contact: null
  organization: APISIX
  project: apisix-ingress-controller
  url: https://github.com/apache/apisix-ingress-controller.git
  version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
    result: partial
    skippedTests:
    - GatewaySecretInvalidReferenceGrant
    - GatewaySecretMissingReferenceGrant
    - HTTPRouteHTTPSListener
    - HTTPRouteInvalidCrossNamespaceBackendRef
    - HTTPRouteInvalidReferenceGrant
    - HTTPRoutePartiallyInvalidViaInvalidReferenceGrant
    - HTTPRouteReferenceGrant
    statistics:
      Failed: 0
      Passed: 26
      Skipped: 7
  name: GATEWAY-HTTP
  summary: Core tests partially succeeded with 7 test skips.

Signed-off-by: ashing <[email protected]>
@ronething ronething requested review from AlinsRan and Copilot May 14, 2025 09:23
Copy link
Contributor

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 aims to fix conformance test issues by reducing the initialization synchronization delay and enhancing the SSL merging logic in the ADC translator. Key changes include:

  • Reducing the InitSyncDelay from 30 minutes to 1 minute within the test suite.
  • Adding debug logging and implementing a merge function for SSL objects with identical IDs.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
test/conformance/suite_test.go Reduced the initialization synchronization delay to speed up test execution.
internal/provider/adc/translator/gateway.go Added debug logs and a function to merge SSL objects with duplicate IDs.
Comments suppressed due to low confidence (1)

test/conformance/suite_test.go:162

  • Consider adding a comment to explain the rationale behind reducing InitSyncDelay from 30 minutes to 1 minute so that future maintainers understand this test-specific configuration.
InitSyncDelay: 1 * time.Minute,

Signed-off-by: ashing <[email protected]>
@ronething ronething changed the title fix: conformance test fix: merge duplicate ssl object when translate gateway May 14, 2025
@ronething ronething requested a review from Copilot May 15, 2025 02:06
Copy link
Contributor

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 fixes an issue where duplicate SSL objects were not merged during gateway translation. Key changes include:

  • Reducing the test suite's sync delay from 30 minutes to 1 minute.
  • Adding a new mergeSSLWithSameID function to consolidate duplicate SSL objects.
  • Introducing debug logging for generated SSL IDs.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
test/conformance/suite_test.go Reduced the initialization sync delay to speed up test execution
internal/provider/adc/translator/gateway.go Added a function to merge duplicate SSL objects and enhanced logging
Comments suppressed due to low confidence (1)

internal/provider/adc/translator/gateway.go:277

  • Consider adding unit tests for the mergeSSLWithSameID function to validate its behavior with various duplicate SSL scenarios.
mergedSSL := make([]*adctypes.SSL, 0, len(sslMap))

}
}

// mergeSSLWithSameID merge ssl with same id
Copy link

Copilot AI May 15, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider revising the function comment to 'Merge SSL objects with identical IDs' for improved clarity.

Copilot uses AI. Check for mistakes.
@ronething ronething merged commit 7e5ab58 into release-v2-dev May 15, 2025
9 checks passed
@ronething ronething deleted the fix/fullsync branch May 15, 2025 03:18
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.

3 participants