-
Couldn't load subscription status.
- Fork 1.6k
📖 Refactor sampleexternalplugin to be a Valid Reference Implementation #5116
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
base: master
Are you sure you want to change the base?
📖 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. Instructions 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nerdeveloper The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
63b82e0 to
edc4046
Compare
Transform the sample external plugin from mock scaffolding to a realistic working example that demonstrates best practices for external plugin development. Changes: - Replace mock init/create api/webhook commands with meaningful edit command - Add Prometheus ServiceMonitor scaffolding for operator monitoring - Implement PROJECT config reading to align with internal plugin patterns - Add local source replace directive for contributor testing - Update documentation to reflect realistic plugin usage The plugin now serves as a proper reference for: - Reading PROJECT config in external plugins - Scaffolding real, production-ready configurations - Testing plugins against local Kubebuilder source - Adding optional features via the edit subcommand
- Change ProjectConfig tags from json to yaml for correct unmarshaling - Unmarshal directly into ProjectConfig struct instead of config.Config interface - Fix fmt.Sprintf template to pass domain twice for both Service and ServiceMonitor labels - Remove unused config import - Tested plugin successfully generates correct Prometheus monitoring files
…ustomization, update PROJECT file handling, and improve test script
…ry intentionally empty
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! |
docs/book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/cmd/flags.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution 🥇
I tried give the first round of reviews, let me know wdyt.
…t init and edit commands, update manifests, and enhance test scripts
There was a problem hiding this 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 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.
| kubebuilder edit --plugins sampleexternalplugin/v1 | ||
|
|
||
| # Ensure Prometheus assets were scaffolded | ||
| test -f config/prometheus/prometheus.yaml |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test checks for prometheus.yaml but the code scaffolds the file at path config/prometheus/prometheus.yaml according to prometheus.go line 47. The filename should be prometheus.yaml, not monitor.yaml as mentioned in the PR description.
| pluginResponse.Error = true | ||
| pluginResponse.ErrorMsgs = []string{ | ||
| "unrecognized command: " + pr.Command, | ||
| "unrecognized subcommand flag in args: " + string(rune(len(pr.Args))), |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| return &ProjectConfig{ | ||
| Domain: "example.com", | ||
| ProjectName: "project", | ||
| Repo: "example.com/project", | ||
| }, nil |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silently returning default values when the PROJECT file doesn't exist could mask configuration issues. For the edit subcommand, which expects to modify an existing project, consider returning an error instead to inform users that they need to run init first.
| return &ProjectConfig{ | |
| Domain: "example.com", | |
| ProjectName: "project", | |
| Repo: "example.com/project", | |
| }, nil | |
| return nil, fmt.Errorf("PROJECT file does not exist; please run 'init' first") |
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