📖 Refactor sampleexternalplugin to be a Valid Reference Implementation#5116
Conversation
|
Hi @nerdeveloper. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
63b82e0 to
edc4046
Compare
d42c0be to
5395def
Compare
|
Hi @nerdeveloper thanks for the PR! This is a large one and will take some time to review. While you wait for the review, could you please adjust the title so it follows the guidelines for PR titles? Also, could you squash the four commits into one just so we keep the rule of a single commit per PR? Thanks! |
camilamacedo86
left a comment
There was a problem hiding this comment.
Thank you for your contribution 🥇
I tried give the first round of reviews, let me know wdyt.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the sampleexternalplugin from mock scaffolding into a realistic Prometheus monitoring generator that demonstrates best practices for external plugin development. The plugin now uses the edit subcommand to add optional monitoring features by reading PROJECT config and generating actual ServiceMonitor YAML manifests.
Key Changes:
- Removed mock
init,create api, andcreate webhooksubcommands in favor of realisticeditsubcommand - Implemented PROJECT config reading using
config.ConfigAPIs similar to internal plugins - Added local source replace directive for testing against local Kubebuilder source
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
v1/scaffolds/init.go |
Updated to scaffold Prometheus monitoring resources during initialization |
v1/scaffolds/edit.go |
New file implementing edit subcommand with PROJECT config loading |
v1/scaffolds/api.go |
Removed - mock create api subcommand deleted |
v1/scaffolds/webhook.go |
Removed - mock create webhook subcommand deleted |
v1/internal/test/plugins/prometheus/prometheus.go |
New Prometheus instance manifest template |
v1/internal/test/plugins/prometheus/kustomization.go |
New kustomization templates for Prometheus resources |
v1/cmd/cmd.go |
Updated command routing to support init and edit only |
v1/cmd/metadata.go |
Updated to handle init and edit metadata requests |
v1/cmd/flags.go |
Refactored flag handling for init and edit subcommands |
v1/go.mod |
Added afero dependency and local source replace directive |
v1/test/test.sh |
Updated test to verify Prometheus asset scaffolding |
external-plugins.md |
Updated documentation with realistic edit command examples |
v1/testdata/testplugin/* |
Removed mock test files |
v1/scaffolds/internal/templates/* |
Removed mock template files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pluginResponse.Error = true | ||
| pluginResponse.ErrorMsgs = []string{ | ||
| "unrecognized command: " + pr.Command, | ||
| "unrecognized subcommand flag in args: " + string(rune(len(pr.Args))), |
There was a problem hiding this comment.
Converting len(pr.Args) to a rune and then to a string produces a Unicode character, not a numeric string. This will result in confusing error messages. Use strconv.Itoa(len(pr.Args)) or fmt.Sprintf(\"%d\", len(pr.Args)) instead.
|
@camilamacedo86 should I also implement the copilot suggestions? |
You need here:
I hope that can helps out. |
9d001d4 to
8c61760
Compare
Transform the sample external plugin from mock scaffolding to a realistic Prometheus monitoring generator that demonstrates best practices for external plugin development. Key changes: - Implement init and edit subcommands that scaffold Prometheus instance manifests - Add PROJECT config reading to align with internal plugin patterns - Replace mock ServiceMonitor with complete Prometheus CR including RBAC - Add --namespace flag support to demonstrate argument passing - Update documentation with realistic examples showing argument usage - Fix error handling to properly validate PROJECT file existence - Add comprehensive kustomization manifests for Prometheus resources The plugin now serves as a proper reference implementation for: - Reading PROJECT configuration in external plugins - Scaffolding production-ready Kubernetes manifests - Supporting command-line arguments (--namespace flag) - Testing plugins against local Kubebuilder source - Adding optional monitoring features via edit subcommand Addresses maintainer feedback on PR kubernetes-sigs#5116 including proper error handling, argument passing examples, and realistic manifest generation. Fixes kubernetes-sigs#4824
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7e2b248 to
9afd683
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Initialize a new project with Prometheus monitoring | ||
| kubebuilder init --plugins sampleexternalplugin/v1 |
There was a problem hiding this comment.
The documentation shows "Initialize a new project with Prometheus monitoring" as an example use case for the sampleexternalplugin, but this doesn't align with the actual test implementation. The test script uses 'kubebuilder init --plugins go/v4' first, then 'kubebuilder edit --plugins sampleexternalplugin/v1'. This suggests the plugin is meant to add Prometheus to an existing project, not during initialization. Either update the documentation example to reflect the actual usage pattern (adding Prometheus to an existing project), or ensure the init subcommand is properly tested and documented.
e7f18f3 to
ba09131
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Can you please rebase with master this one? |
Transform the sample external plugin from mock scaffolding to a realistic Prometheus monitoring generator that demonstrates best practices for external plugin development. Key changes: - Implement init and edit subcommands that scaffold Prometheus instance manifests - Add PROJECT config reading to align with internal plugin patterns - Replace mock ServiceMonitor with complete Prometheus CR including RBAC - Add --namespace flag support to demonstrate argument passing - Update documentation with realistic examples showing argument usage - Fix error handling to properly validate PROJECT file existence - Centralize kustomization in config/default/kustomization.yaml (no separate prometheus kustomization) - Provide clear instructions for users to add Prometheus resources manually The plugin now serves as a proper reference implementation for: - Reading PROJECT configuration in external plugins - Scaffolding production-ready Kubernetes manifests - Supporting command-line arguments (--namespace flag) - Testing plugins against local Kubebuilder source - Adding optional monitoring features via edit subcommand - Following Kubebuilder's centralized kustomization pattern Addresses maintainer feedback on PR 5116 including proper error handling, argument passing examples, realistic manifest generation, and centralized kustomization.
f1cb16c to
eb76444
Compare
|
Hi @camilamacedo86, Done! I've rebased the branch with the latest master and resolved the go.mod/go.sum conflicts. Summary:
Ready for your review. Thanks! |
camilamacedo86
left a comment
There was a problem hiding this comment.
/lgtm
Great work 🎉
/ok-to-test
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, nerdeveloper The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary
This PR refactors the
sampleexternalpluginfrom outdated mock scaffolding into a realistic, working reference implementation that demonstrates best practices for external plugin development.Problem
The current
sampleexternalplugin(issue #4824) had several issues:initFile.txt,apiFile.txt)Solution
Transformed the plugin into a Prometheus Monitoring Generator that:
editsubcommand to add optional Prometheus monitoring featuresChanges
Plugin Functionality
init,create api,create webhooksubcommandseditsubcommand that scaffolds Prometheus monitoringconfig/prometheus/monitor.yaml- ServiceMonitor resourceconfig/prometheus/kustomization.yaml- Kustomize configconfig/default/kustomization_prometheus_patch.yaml- Integration instructionsCode Quality
config.ConfigAPIs like internal pluginsreplace sigs.k8s.io/kubebuilder/v4 => ../../../../../../../Documentation
external-plugins.mdwith realisticeditcommand examplesUsage Example
# Add Prometheus monitoring to your operator project kubebuilder edit --plugins sampleexternalplugin/v1This will scaffold:
Testing
Impact
This PR makes the sample plugin a proper reference that:
Fixes #4824