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

titpetric commented 1 month ago

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

github-actions[bot] commented 1 month ago

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.

github-actions[bot] commented 1 month ago

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

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	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]](https://github.com/TykTechnologies/tyk/pull/6604/files#diff-132e755f4956d897a7efcd7d471019732e0ab425aa39780041aee0ab1d653467R23-R23) ```diff -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]](https://github.com/TykTechnologies/tyk/pull/6604/files#diff-a1bd10b604998b1dec2b7ecbb559f497cf310ac2e02e4078a61a0a7f70b4f428R12-R12) ```diff -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
Enhancement	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]](https://github.com/TykTechnologies/tyk/pull/6604/files#diff-21c9d908acf40538dfdc96f947df535a9d18e088dac226260ac0be8b7ab0452fR1-R2) ```diff //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

sonarcloud[bot] commented 1 month ago

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

TykTechnologies / tyk