-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
🌱 (phase 2 plugins): add simple external plugin to testdata #2810
🌱 (phase 2 plugins): add simple external plugin to testdata #2810
Conversation
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
/hold I don't want this to merge until there has been more discussion on this change. I feel it may need some more work. |
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
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.
Just a few nits. Otherwise it shows terrific 🥇
Thank you !
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
/retest |
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@camilamacedo86 @rashmigottipati Would you mind reviewing this again when you have some time? Thanks! |
@@ -0,0 +1,46 @@ | |||
/* |
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.
Could we:
- Add the plugin inside of a directory called tesdata? Why? In Golang it is a good practice to know that is a mock/sample. So that would be
docs/book/src/simple-external-plugin-tutorial/testdata/
dir instead - Could we have a better layout for this sample , i.e.:
docs/book/src/simple-external-plugin-tutorial/testdata/
|--- pkg
| ---> we add here api.go/ init.go, helpers.go and etc
- then we have under main.go, go.mod, go.sum
Also, have we not templates in this plugin?
If so, could we have them instead of templates dir much more similar to how we structure our plugin? Look the pkg/plugins/golang/v3
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.
@camilamacedo86 I updated the structure of the sample external plugin to be more similar to the existing Kubebuilder plugins in commit ee04eee
Let me know what you think!
@@ -0,0 +1,46 @@ | |||
/* | |||
Copyright 2022 The Kubernetes Authors. | |||
|
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.
Could we add here a Makefile with a target to build the plugin in the .bin/ dir ( we need ignore this dir as well )
Then, could we add an e2e test that just creates a project using the plugin and test it out against the Kind?
That would be here: kubebuilder/test/e2e/external-plugin/
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.
I feel like this would be best in a follow up. I was hoping to keep the scope of this PR smaller (i.e just adding the sample) and add the e2e testing components in a later PR (I think @rashmigottipati mentioned that she had already started on some e2e testing work).
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 idea here would ensure that the sample / mock works and then ideally would be nice to be have both at the same PR. However, I am OK with us working on this one as a follow-up.
Therefore, if @rashmigottipati is doing that probably is not with this sample and then we could have here another one. ( her python example ) also tested in the e2e and used in the docs. WDYT is that make sense?
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.
@camilamacedo86 I added a Makefile and a target named build
that when run will build the sample external plugin and output the binary to ./bin/sampleexternalplugin
in commit ee04eee
If we still want to do a simple e2e test to ensure that the phase 2 plugins are working properly with this sample plugin I can create a simple test to ensure that the proper files are created and contain the right contents after running scaffold commands with the sample plugin. Unfortunately this sample doesn't scaffold anything that can actually be run on a cluster and just produces text files.
Just wanted to post an update: |
to be structured more like existing Kubebuilder plugins. Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
to be structured more like existing Kubebuilder plugins. Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
/retest |
/unhold |
/cc @jmrodri |
SHELL = /bin/bash | ||
|
||
build: | ||
go build -o ./bin/sampleexternalplugin |
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.
nit just an space
} | ||
|
||
const WebhookFileDefaultMessage = "A simple text file created with the `create webhook` subcommand" | ||
const WebhookFileHookedMessage = "\nHOOKED!" |
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.
In this example we are scaffolding a new file when we call the webhook command with, Am I right?
So, wdyt about a sample that can be called in the init or create api to be something more simple?
Note that to scaffold the webhook we need to have a resource created first.
Also, the sample could for example just add a file when we create the project or replace/add content when we call create api for example.
Make update the Makefile and add a new target for example, 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.
Hi @everettraven,
Since it is an initial version of the testdata sample as we spoke I think we can move forward here to make eaiser the process to add the sample, docs, and e2e tests instead of trying to do it all in the same PR. Therefore, how you agree to work on the following when possible shows all fine.
Thank you for the terrific work 🥇
So,
/lgtm
/approved
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description of the change
Add a new project to the
testdata/
directory that is a very simple Phase 2 Plugin written in Go.The implementation also includes comments that could be helpful for those attempting to write their own Phase 2 Plugins.
Motivation for the change
Create a sample Phase 2 Plugin that can be used to help with e2e testing the Phase 2 Plugins implementation.