Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propose removing goplugin tags in favor of cgo build tag (implicit). #6604

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

titpetric
Copy link
Contributor

@titpetric titpetric commented Oct 5, 2024

User description

Plugins require compilation with CGO_ENABLED=1; build tag adds unnecessary flags to the tests and build process.

# CGO_ENABLED=0 go build .
# file tyk
tyk: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), statically linked, Go BuildID=pSVrnN1LVmossojPNzFI/QDujFlt6OoVDMabdtOOZ/Z7FxIh4goB2mjtj-yoxN/X_r8d4PyVfYy_fu6C2Un, with debug_info, not stripped

Verified with:

# ./tyk plugin load -f x
tyk: error: unexpected error: goplugin.GetSymbol is disabled, build gateway with CGO_ENABLED=1, try --help

When enabled:

# CGO_ENABLED=1 go build .
root@carbon:~/tyk/tyk# file tyk
tyk: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=3c80750859fafdef3a34f630d272a78e50f29583, for GNU/Linux 3.2.0, with debug_info, not stripped

Verified with:

# ./tyk plugin load -f x
tyk: error: unexpected error: plugin.Open("x"): realpath failed, try --help

Acceptance:

  • Requires gomit to forget about the goplugin tag (goreleaser config change). Follow up with PR.
  • Requires a more particular build tag to match supported plugin platforms, CGO has wider support than plugins do

PR Type

enhancement, configuration changes


Description

  • Replaced goplugin build tags with cgo build tags across multiple Go files to align with CGO requirements.
  • Updated error messages to reflect the new build tag usage.
  • Removed goplugin tag from CI test scripts and goreleaser configurations to streamline the build process.
  • Added comments in the CI test script regarding its current usage status.

Changes walkthrough 📝

Relevant files
Enhancement
analyticsplugin.go
Replace goplugin build tags with cgo in analyticsplugin   

goplugin/analyticsplugin.go

  • Replaced goplugin build tags with cgo build tags.
+2/-2     
goplugin.go
Replace goplugin build tags with cgo in goplugin                 

goplugin/goplugin.go

  • Replaced goplugin build tags with cgo build tags.
+2/-2     
mw_go_plugin_test.go
Replace goplugin build tags with cgo in test file               

goplugin/mw_go_plugin_test.go

  • Replaced goplugin build tags with cgo build tags.
+2/-2     
no_analyticsplugin.go
Replace !goplugin build tags with !cgo in no_analyticsplugin

goplugin/no_analyticsplugin.go

  • Replaced !goplugin build tags with !cgo build tags.
  • Updated error message to reflect new build tag.
  • +2/-3     
    no_goplugin.go
    Replace !goplugin build tags with !cgo in no_goplugin       

    goplugin/no_goplugin.go

  • Replaced !goplugin build tags with !cgo build tags.
  • Updated error message to reflect new build tag.
  • +3/-3     
    Configuration changes
    ci-tests.sh
    Remove goplugin tag from CI test script                                   

    bin/ci-tests.sh

  • Removed goplugin tag from CI test script.
  • Added comments about the script's current usage status.
  • +4/-1     
    goreleaser-5.0.yml
    Remove goplugin tag from goreleaser-5.0 configuration       

    ci/goreleaser/goreleaser-5.0.yml

    • Removed goplugin tag from build configurations.
    +0/-4     
    goreleaser.yml
    Remove goplugin tag from goreleaser configuration               

    ci/goreleaser/goreleaser.yml

    • Removed goplugin tag from multiple build configurations.
    +0/-4     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    github-actions bot commented Oct 5, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Message Update
    The error message in no_goplugin.go was updated to suggest enabling CGO, but it might still be misleading if CGO is enabled and the plugin functionality is not available due to other reasons. Consider refining the error message to cover more scenarios or provide clearer guidance.

    Copy link
    Contributor

    github-actions bot commented Oct 5, 2024

    API Changes

    --- prev.txt	2024-10-05 11:04:34.508925219 +0000
    +++ current.txt	2024-10-05 11:04:28.472937700 +0000
    @@ -10866,13 +10866,13 @@
     FUNCTIONS
     
     func GetAnalyticsHandler(path string, symbol string) (func(record *analytics.AnalyticsRecord), error)
    -func GetHandler(path string, symbol string) (http.HandlerFunc, error)
    +func GetHandler(modulePath string, symbol string) (http.HandlerFunc, error)
     func GetPluginFileNameToLoad(pluginStorage storage, pluginPath string) (string, error)
         GetPluginFileNameToLoad check which file to load based on name, tyk version,
         os and architecture but it also takes care of returning the name of the file
         that exists
     
    -func GetResponseHandler(path string, symbol string) (func(rw http.ResponseWriter, res *http.Response, req *http.Request), error)
    +func GetResponseHandler(modulePath string, symbol string) (func(rw http.ResponseWriter, res *http.Response, req *http.Request), error)
     func GetSymbol(modulePath string, symbol string) (interface{}, error)
     
     TYPES

    Copy link
    Contributor

    github-actions bot commented Oct 5, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Ensure build tags in CI scripts are updated to match new requirements

    Update the build tags in the CI script to explicitly include 'cgo' to ensure
    consistency with the new build requirements.

    bin/ci-tests.sh [23]

    -tags="dev"
    +tags="cgo dev"
    Suggestion importance[1-10]: 8

    Why: Updating the build tags to include 'cgo' aligns the CI script with the new build requirements, ensuring consistency and preventing potential build failures.

    8
    Enhancement
    Improve the clarity of the error message regarding CGO enabling

    Replace the error message to include more specific instructions about enabling CGO,
    as the current message might be unclear to users.

    goplugin/no_goplugin.go [12]

    -errNotImplemented = "goplugin.%s is disabled, build gateway with CGO_ENABLED=1"
    +errNotImplemented = "goplugin.%s is disabled, please ensure CGO is enabled by setting CGO_ENABLED=1"
    Suggestion importance[1-10]: 7

    Why: The suggestion improves the clarity of the error message, making it more explicit about how to enable CGO. This can help users understand the necessary steps to resolve the issue.

    7
    Add conditional compilation errors for clarity and immediate developer feedback

    Consider adding a conditional compilation error or warning if the 'cgo' tag is not
    enabled, to provide immediate feedback during development.

    goplugin/no_analyticsplugin.go [1-2]

     //go:build !cgo
     // +build !cgo
    +#error "CGO must be enabled for this module."
    Suggestion importance[1-10]: 6

    Why: Adding a compilation error provides immediate feedback if CGO is not enabled, which can be useful during development. However, it might be too restrictive for some use cases, so it should be considered carefully.

    6

    Copy link

    sonarqubecloud bot commented Oct 5, 2024

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant