Skip to content

Conversation

@nerdeveloper
Copy link
Contributor

@nerdeveloper nerdeveloper commented Oct 5, 2025

Summary

This PR refactors the sampleexternalplugin from 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:

  • ❌ Mock scaffolding that created useless text files (initFile.txt, apiFile.txt)
  • ❌ Didn't use PROJECT config or align with internal plugin architecture
  • ❌ Used release module instead of local source for testing
  • ❌ Not a realistic example for plugin developers

Solution

Transformed the plugin into a Prometheus Monitoring Generator that:

  • ✅ Uses edit subcommand to add optional Prometheus monitoring features
  • ✅ Reads PROJECT config for domain and project metadata
  • ✅ Generates real ServiceMonitor YAML manifests
  • ✅ Uses local Kubebuilder source via replace directive
  • ✅ Follows internal plugin architecture patterns

Changes

Plugin Functionality

  • Removed: Mock init, create api, create webhook subcommands
  • Added: Realistic edit subcommand that scaffolds Prometheus monitoring
  • Implements: PROJECT config reading (domain, projectName, repo)
  • Generates:
    • config/prometheus/monitor.yaml - ServiceMonitor resource
    • config/prometheus/kustomization.yaml - Kustomize config
    • config/default/kustomization_prometheus_patch.yaml - Integration instructions

Code Quality

  • Uses config.Config APIs like internal plugins
  • Proper Universe pattern for file scaffolding
  • Local source replace directive: replace sigs.k8s.io/kubebuilder/v4 => ../../../../../../../

Documentation

  • Updated external-plugins.md with realistic edit command examples
  • Changed examples from mock init/api/webhook to real Prometheus monitoring use case

Usage Example

# Add Prometheus monitoring to your operator project
kubebuilder edit --plugins sampleexternalplugin/v1

This will scaffold:

  • Prometheus ServiceMonitor configuration
  • Kustomize resources for monitoring
  • Integration instructions

Testing

  • ✅ Plugin builds successfully with local source
  • ✅ All mock files removed, real templates in place
  • ✅ Documentation reflects new usage

Impact

This PR makes the sample plugin a proper reference that:

  • Shows how to read PROJECT config in external plugins
  • Demonstrates real-world use case (optional monitoring features)
  • Allows contributors to test against local Kubebuilder source
  • Aligns with internal plugin architecture patterns

Fixes #4824

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 5, 2025
@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 5, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nerdeveloper
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nerdeveloper nerdeveloper force-pushed the refactor-sampleexternalplugin-issue-4824 branch from 63b82e0 to edc4046 Compare October 5, 2025 14:51
@nerdeveloper nerdeveloper changed the title ✨ Refactor sampleexternalplugin to be a Valid Reference Implementation Refactor sampleexternalplugin to be a Valid Reference Implementation Oct 5, 2025
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 5, 2025
@nerdeveloper nerdeveloper force-pushed the refactor-sampleexternalplugin-issue-4824 branch from d42c0be to 5395def Compare October 5, 2025 16:48
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Oct 5, 2025
@vitorfloriano
Copy link
Contributor

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!

Copy link
Member

@camilamacedo86 camilamacedo86 left a 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.

@camilamacedo86 camilamacedo86 changed the title Refactor sampleexternalplugin to be a Valid Reference Implementation 📖 Refactor sampleexternalplugin to be a Valid Reference Implementation Oct 27, 2025
Copy link

Copilot AI left a 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, and create webhook subcommands in favor of realistic edit subcommand
  • Implemented PROJECT config reading using config.Config APIs 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
Copy link

Copilot AI Oct 27, 2025

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.

Copilot uses AI. Check for mistakes.
pluginResponse.Error = true
pluginResponse.ErrorMsgs = []string{
"unrecognized command: " + pr.Command,
"unrecognized subcommand flag in args: " + string(rune(len(pr.Args))),
Copy link

Copilot AI Oct 27, 2025

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.

Copilot uses AI. Check for mistakes.
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2025
@nerdeveloper
Copy link
Contributor Author

@camilamacedo86 should I also implement the copilot suggestions?

@camilamacedo86
Copy link
Member

camilamacedo86 commented Nov 7, 2025

Hi @nerdeveloper

@camilamacedo86 should I also implement the copilot suggestions?

You need here:

  1. Rebase the PR with master -> Ensure that the example cover the latest changes
  2. Address all comments / reviews that make sense -> We have been using AI to help out, so if the comment/suggestion made by Copilot make sense we should address as well
  3. Ensure that all pass on the CI/tests
  4. Ensure that we have only 1 commit for the PR
  5. Ensure that the changes made in the sample are accurate, will better clarify how to extend the lib to create external type plugins with a more practical / valid use case scenario.

I hope that can helps out.
Thank you for looking on that 🎉

@nerdeveloper nerdeveloper force-pushed the refactor-sampleexternalplugin-issue-4824 branch from 9d001d4 to 8c61760 Compare November 10, 2025 03:56
@nerdeveloper nerdeveloper force-pushed the refactor-sampleexternalplugin-issue-4824 branch 2 times, most recently from 5b04b5e to 036056e Compare November 10, 2025 04:14
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Nov 10, 2025
Copy link

Copilot AI left a 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{
Copy link

Copilot AI Nov 10, 2025

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.

Copilot uses AI. Check for mistakes.
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2025
@nerdeveloper nerdeveloper force-pushed the refactor-sampleexternalplugin-issue-4824 branch from 036056e to 3aa53d1 Compare December 8, 2025 01:53
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2025
@nerdeveloper
Copy link
Contributor Author

@camilamacedo86 just waiting for your approval

@camilamacedo86
Copy link
Member

Hi @nerdeveloper

Really amazing work 🎉
I think it is just missing address the few comments raised by Copilot and that is it
We can get this one merged

Thank you a lot for all your effort on this one!!!!

@nerdeveloper nerdeveloper force-pushed the refactor-sampleexternalplugin-issue-4824 branch from 3aa53d1 to b5fbffe Compare January 5, 2026 03:55
@nerdeveloper
Copy link
Contributor Author

Hi @camilamacedo86,

I've addressed the Copilot suggestions:

  1. Fixed: metadata.go:68-69 - Changed the vague error message from "unrecognized flag" to "no subcommand flag provided; expected --init or --edit" (commit b5fbffe)

  2. False positive: The Copilot comment on external-plugins.md:100 about config/prometheus/kustomization.yaml not being scaffolded is incorrect. The implementation does scaffold this file:

    • kustomization.go:27 defines Path: "config/prometheus/kustomization.yaml"
    • edit.go:69 creates prometheusKustomization := prometheus.NewPrometheusKustomization()
    • edit.go:76 adds it to universe: pluginResponse.Universe[prometheusKustomization.Path] = prometheusKustomization.Content

The documentation is accurate and matches the implementation.

Thanks!

Copy link

Copilot AI left a 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.

Comment on lines 45 to 47
projectConfig := &ProjectConfig{
Domain: "example.com",
ProjectName: "project",
Repo: "example.com/project",
}
Copy link

Copilot AI Jan 5, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines +63 to +76
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
`
Copy link

Copilot AI Jan 5, 2026

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.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI left a 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.

Comment on lines 166 to 167
# Initialize a new project with Prometheus monitoring
kubebuilder init --plugins sampleexternalplugin/v1
Copy link

Copilot AI Jan 7, 2026

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.

Copilot uses AI. Check for mistakes.
const defaultKustomizationPatchTemplate = `# [PROMETHEUS] To enable prometheus monitoring, add the following to config/default/kustomization.yaml:
#
# In the resources section, add:
# - ../prometheus/prometheus.yaml
Copy link

Copilot AI Jan 7, 2026

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.

Suggested change
# - ../prometheus/prometheus.yaml
# - ../prometheus

Copilot uses AI. Check for mistakes.
Comment on lines 82 to 83
type ProjectConfig struct {
Domain string
ProjectName string
Copy link

Copilot AI Jan 7, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines 33 to 35
# Clean up test files
cd ../..
rm -rf testdata/testplugin
Copy link

Copilot AI Jan 7, 2026

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.

Copilot uses AI. Check for mistakes.
}
}

const defaultKustomizationPatchTemplate = `# [PROMETHEUS] To enable prometheus monitoring, add the following to config/default/kustomization.yaml:
Copy link

Copilot AI Jan 7, 2026

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.

Suggested change
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:

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI left a 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.

@nerdeveloper nerdeveloper force-pushed the refactor-sampleexternalplugin-issue-4824 branch from 9afd683 to e7f18f3 Compare January 7, 2026 10:07
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.
@nerdeveloper nerdeveloper force-pushed the refactor-sampleexternalplugin-issue-4824 branch from e7f18f3 to ba09131 Compare January 7, 2026 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make sampleexternalplugin a Valid Reference Implementation

4 participants