-
Notifications
You must be signed in to change notification settings - 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. 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. |
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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! |
docs/book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/cmd/flags.go
Show resolved
Hide resolved
camilamacedo86
left a comment
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.
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.
.../book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/scaffolds/edit.go
Outdated
Show resolved
Hide resolved
...-tutorial/testdata/sampleexternalplugin/v1/internal/test/plugins/prometheus/kustomization.go
Show resolved
Hide resolved
|
@camilamacedo86 should I also implement the copilot suggestions? |
You need here:
I hope that can helps out. |
9d001d4 to
8c61760
Compare
5b04b5e to
036056e
Compare
...gin-tutorial/testdata/sampleexternalplugin/v1/internal/test/plugins/prometheus/prometheus.go
Outdated
Show resolved
Hide resolved
...gin-tutorial/testdata/sampleexternalplugin/v1/internal/test/plugins/prometheus/prometheus.go
Outdated
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.
Pull Request Overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| return pluginResponse | ||
| // For init command, we'll use default values since PROJECT file may not exist yet | ||
| projectConfig := &ProjectConfig{ |
Copilot
AI
Nov 10, 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 ProjectConfig type is used here but is only defined in edit.go. This will cause a compilation error. Either move the ProjectConfig type definition to a shared location or duplicate it in this file.
.../book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/scaffolds/edit.go
Outdated
Show resolved
Hide resolved
...gin-tutorial/testdata/sampleexternalplugin/v1/internal/test/plugins/prometheus/prometheus.go
Outdated
Show resolved
Hide resolved
docs/book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/cmd/metadata.go
Show resolved
Hide resolved
docs/book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/test/test.sh
Show resolved
Hide resolved
.../book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/scaffolds/edit.go
Outdated
Show resolved
Hide resolved
036056e to
3aa53d1
Compare
|
@camilamacedo86 just waiting for your approval |
|
Really amazing work 🎉 Thank you a lot for all your effort on this one!!!! |
3aa53d1 to
b5fbffe
Compare
|
Hi @camilamacedo86, I've addressed the Copilot suggestions:
The documentation is accurate and matches the implementation. Thanks! |
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
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.
docs/book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/go.mod
Outdated
Show resolved
Hide resolved
| projectConfig := &ProjectConfig{ | ||
| Domain: "example.com", | ||
| ProjectName: "project", | ||
| Repo: "example.com/project", | ||
| } |
Copilot
AI
Jan 5, 2026
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 type ProjectConfig is used here but is defined in edit.go. Since both init.go and edit.go are in the same package (scaffolds), this will work, but it creates unclear coupling. Consider either: 1) moving ProjectConfig and loadProjectConfig to a shared file (e.g., config.go), or 2) defining ProjectConfig in init.go since it's simpler there and doesn't need the loadProjectConfig function, or 3) extracting it to a common location if both files need it.
...gin-tutorial/testdata/sampleexternalplugin/v1/internal/test/plugins/prometheus/prometheus.go
Outdated
Show resolved
Hide resolved
| const prometheusTemplate = `# Prometheus Instance | ||
| # This creates a Prometheus instance that will scrape metrics from your operator. | ||
| # Requires the Prometheus Operator to be installed in your cluster. | ||
| # See: https://github.com/prometheus-operator/prometheus-operator | ||
| apiVersion: monitoring.coreos.com/v1 | ||
| kind: Prometheus | ||
| metadata: | ||
| name: prometheus | ||
| labels: | ||
| app.kubernetes.io/name: %s | ||
| app.kubernetes.io/managed-by: kustomize | ||
| spec: | ||
| replicas: 1 | ||
| scrapeInterval: 30s | ||
| serviceAccountName: controller-manager | ||
| serviceMonitorSelector: | ||
| matchLabels: | ||
| control-plane: controller-manager | ||
| securityContext: | ||
| runAsNonRoot: true | ||
| seccompProfile: | ||
| type: RuntimeDefault | ||
| ` |
Copilot
AI
Jan 5, 2026
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 PR description states that the plugin generates "config/prometheus/monitor.yaml - ServiceMonitor resource", but the actual code generates a Prometheus CR (Custom Resource) with kind "Prometheus", not a ServiceMonitor. This is a discrepancy between documentation and implementation. Either update the PR description or change the implementation to match what was described.
.../book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/scaffolds/edit.go
Outdated
Show resolved
Hide resolved
7e2b248 to
9afd683
Compare
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
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 |
Copilot
AI
Jan 7, 2026
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 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.
| const defaultKustomizationPatchTemplate = `# [PROMETHEUS] To enable prometheus monitoring, add the following to config/default/kustomization.yaml: | ||
| # | ||
| # In the resources section, add: | ||
| # - ../prometheus/prometheus.yaml |
Copilot
AI
Jan 7, 2026
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 instruction comment references "../prometheus/prometheus.yaml" but the kustomization.yaml template (line 33) references "prometheus.yaml" without the path. This creates confusion about the correct resource path. If the Prometheus kustomization is in "config/prometheus/kustomization.yaml", it should reference "prometheus.yaml" (same directory), but the instruction comment suggests "../prometheus/prometheus.yaml" which would be appropriate for "config/default/kustomization.yaml". Consider clarifying the comment to indicate it's for the default kustomization file, not the prometheus one.
| # - ../prometheus/prometheus.yaml | |
| # - ../prometheus |
| type ProjectConfig struct { | ||
| Domain string | ||
| ProjectName string |
Copilot
AI
Jan 7, 2026
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 Domain field is extracted from PROJECT config but is never used in the edit command. Either remove this unused field from ProjectConfig, or use it in the generated Prometheus manifests if it's intended for future use. Currently, only ProjectName is used in the Prometheus templates.
| # Clean up test files | ||
| cd ../.. | ||
| rm -rf testdata/testplugin |
Copilot
AI
Jan 7, 2026
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 script removes the test directory at the end, which means if the tests fail, the generated artifacts are cleaned up and cannot be inspected for debugging. Consider only removing the test directory on success, or adding a flag to preserve it for debugging purposes.
| } | ||
| } | ||
|
|
||
| const defaultKustomizationPatchTemplate = `# [PROMETHEUS] To enable prometheus monitoring, add the following to config/default/kustomization.yaml: |
Copilot
AI
Jan 7, 2026
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 spelling of "prometheus" should be capitalized as "Prometheus" in the comment. The phrase should read "[PROMETHEUS] To enable Prometheus monitoring" for consistency with the rest of the documentation and comments which capitalize "Prometheus" as a proper noun.
| const defaultKustomizationPatchTemplate = `# [PROMETHEUS] To enable prometheus monitoring, add the following to config/default/kustomization.yaml: | |
| const defaultKustomizationPatchTemplate = `# [PROMETHEUS] To enable Prometheus monitoring, add the following to config/default/kustomization.yaml: |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
9afd683 to
e7f18f3
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 - 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.
e7f18f3 to
ba09131
Compare
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