TykTechnologies / tyk

Tyk Open Source API Gateway written in Go, supporting REST, GraphQL, TCP and gRPC protocols
Other
9.68k stars 1.08k forks source link

Merging to release-5.6.1: TT-13130 update gorpc version (#6644) #6646

Closed buger closed 1 week ago

buger commented 1 week ago

User description

TT-13130 update gorpc version (#6644)

User description

TT-13130
Summary Tyk Cloud: Panic appears when a user tried to deploy GW before Control Plane is in deployed state
Type Bug Bug
Status In Dev
Points N/A
Labels Re_open, QA_Fail

Description

Related Issue

TT-13130

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Checklist


PR Type

Bug fix, Enhancement


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
rpc_client.go
Update connection handling in RPC client                                 

rpc/rpc_client.go
  • Replaced ConnectionDialingWG.Wait() with WaitForConnection().
  • Improved connection handling logic.
  • +1/-1     
    Dependencies
    go.mod
    Update gorpc dependency version in go.mod                               

    go.mod - Updated `gorpc` dependency version.
    +1/-1     
    go.sum
    Update go.sum with new gorpc checksums                                     

    go.sum - Added new checksum entries for updated `gorpc` version.
    +2/-0     

    ๐Ÿ’ก PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information


    Co-authored-by: sredny buitrago sredny@srednys-MacBook-Pro.local


    PR Type

    Bug fix, Enhancement


    Description


    Changes walkthrough ๐Ÿ“

    Relevant files
    Enhancement
    rpc_client.go
    Update connection handling in RPC client                                 

    rpc/rpc_client.go
  • Replaced ConnectionDialingWG.Wait() with WaitForConnection().
  • Improved connection handling logic.
  • +1/-1     
    Dependencies
    go.mod
    Update gorpc dependency version in go.mod                               

    go.mod - Updated `gorpc` dependency version.
    +1/-1     
    go.sum
    Update go.sum with new gorpc checksums                                     

    go.sum - Added new checksum entries for updated `gorpc` version.
    +2/-2     

    ๐Ÿ’ก PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    github-actions[bot] commented 1 week ago

    PR Reviewer Guide ๐Ÿ”

    Here are some key observations to aid the review process:

    **๐ŸŽซ Ticket compliance analysis โœ…** **[6644](https://github.com/TykTechnologies/tyk/issues/6644) - Fully compliant** Fully compliant requirements: - Update the gorpc library to a newer version. - Modify the RPC client connection handling to improve connection logic.
    โฑ๏ธ Estimated effort to review: 2 ๐Ÿ”ต๐Ÿ”ตโšชโšชโšช
    ๐Ÿงช No relevant tests
    ๐Ÿ”’ No security concerns identified
    โšก Recommended focus areas for review

    Connection Handling
    Ensure that the new connection handling method `WaitForConnection()` correctly manages all connection scenarios without causing deadlocks or delays in system response.
    github-actions[bot] commented 1 week ago

    API Changes

    no api changes detected
    github-actions[bot] commented 1 week ago

    PR Code Suggestions โœจ

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add error handling for the WaitForConnection method to improve robustness ___ **Consider handling potential errors returned by clientSingleton.WaitForConnection()
    to ensure robust error management and avoid runtime panics.** [rpc/rpc_client.go [309]](https://github.com/TykTechnologies/tyk/pull/6646/files#diff-3b88914c99bb9418e44e6389ce73579843562e8900730b380d7fff2e95c51033R309-R309) ```diff -clientSingleton.WaitForConnection() +if err := clientSingleton.WaitForConnection(); err != nil { + // handle error, possibly log it or retry connection +} ```
    Suggestion importance[1-10]: 9 Why: The suggestion to handle potential errors from `clientSingleton.WaitForConnection()` is highly relevant as it addresses a possible bug by preventing runtime panics and ensuring robust error management. This change significantly enhances the reliability of the code.
    9
    sonarcloud[bot] commented 1 week ago

    Quality Gate Failed Quality Gate failed

    Failed conditions
    0.0% Coverage on New Code (required โ‰ฅ 80%)

    See analysis details on SonarCloud