Skip to content
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.

Sign up for GitHub

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

Jump to bottom

✨Plugin phase 2 implementation #2338

Merged
merged 1 commit into from
May 13, 2022
Merged

✨Plugin phase 2 implementation #2338

merged 1 commit into from
May 13, 2022

Conversation

rashmigottipati
Copy link
Contributor

  • a description of the change
    Adds implementation to discover and run external plugins in a chained manner.

  • the motivation for the change
    Phase 2 Design Proposal

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 15, 2021
@rashmigottipati
Copy link
Contributor Author

/cc @estroz @jmrodri

@k8s-ci-robot
Copy link
Contributor

@rashmigottipati: GitHub didn't allow me to request PR reviews from the following users: jmrodri.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @estroz @jmrodri

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/test-infra repository.

estroz
estroz previously requested changes Sep 22, 2021
View reviewed changes
pkg/cli/options.go Outdated Show resolved Hide resolved
pkg/cli/options.go Outdated Show resolved Hide resolved
pkg/cli/options.go Outdated Show resolved Hide resolved
pkg/cli/options.go Outdated Show resolved Hide resolved
pkg/cli/options.go Outdated Show resolved Hide resolved
pkg/plugins/external/api.go Show resolved Hide resolved
pkg/plugins/external/api.go Outdated Show resolved Hide resolved
pkg/plugins/external/api.go Outdated Show resolved Hide resolved
pkg/plugins/external/api.go Outdated Show resolved Hide resolved
pkg/plugins/external/api.go Outdated Show resolved Hide resolved
jmrodri
jmrodri requested changes Sep 23, 2021
View reviewed changes
Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round pass is done. I'll take another look after you've address Eric's and my comments.

LICENSE Outdated Show resolved Hide resolved
pkg/cli/options.go Outdated Show resolved Hide resolved
pkg/cli/options.go Outdated Show resolved Hide resolved
pkg/cli/options.go Outdated Show resolved Hide resolved
pkg/cli/options.go Outdated Show resolved Hide resolved
pkg/plugins/external/edit.go Outdated Show resolved Hide resolved
pkg/plugins/external/edit.go Outdated Show resolved Hide resolved
pkg/plugins/external/init.go Outdated Show resolved Hide resolved
pkg/plugins/external/webhook.go Outdated Show resolved Hide resolved
pkg/plugins/external/webhook.go Outdated Show resolved Hide resolved
@rashmigottipati rashmigottipati force-pushed the plugin-phase-2 branch 4 times, most recently from eb2184d to 00272bf Compare September 24, 2021 16:01
estroz
estroz reviewed Sep 24, 2021
View reviewed changes
pkg/plugins/external/api.go Outdated Show resolved Hide resolved
estroz
estroz reviewed Sep 24, 2021
View reviewed changes
pkg/cli/options.go Outdated Show resolved Hide resolved
@rashmigottipati rashmigottipati force-pushed the plugin-phase-2 branch 2 times, most recently from 2977788 to 43df417 Compare September 24, 2021 16:53
estroz
estroz reviewed Sep 24, 2021
View reviewed changes
pkg/cli/options.go Outdated Show resolved Hide resolved
estroz
estroz reviewed Sep 24, 2021
View reviewed changes
Copy link
Contributor

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is pretty close to done. I'd like to see a bunch of unit tests and 1 e2e test using the POC code prior to merging.

@camilamacedo86 I'll be on PTO next week so if you could review/approve/lgtm this PR that'd be awesome!

@estroz estroz dismissed their stale review September 24, 2021 17:44

close to done

@rashmigottipati
Copy link
Contributor Author

@estroz thanks a lot for the review. Will work on adding some unit tests and 1 e2e test and update the PR when done.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2021
camilamacedo86
camilamacedo86 reviewed Oct 17, 2021
View reviewed changes
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.

@rashmigottipati could we squash the commits and fix the lint issues?
See that make lint-fix might help you out here.

@camilamacedo86 camilamacedo86 added this to the 3.3.0 milestone Dec 16, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2022
@camilamacedo86 camilamacedo86 modified the milestones: 3.3.0, 3.4.0 Feb 23, 2022
camilamacedo86
camilamacedo86 reviewed Mar 8, 2022
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rashmigottipati,

Some nits for now and follow up : https://github.com/kubernetes-sigs/kubebuilder/pull/2338/files#r872547809

@rashmigottipati rashmigottipati mentioned this pull request Apr 4, 2022
Closed
6 tasks
@rashmigottipati rashmigottipati force-pushed the plugin-phase-2 branch 2 times, most recently from 2fc1502 to f74628f Compare May 13, 2022 14:30
camilamacedo86
camilamacedo86 approved these changes May 13, 2022
View reviewed changes
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.

That is a terrific work 🥇
I loved your sample .. We must have the python plugin here. That is amazing

/approved
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2022
@camilamacedo86
Copy link
Member

/hold

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, rashmigottipati

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2022
@rashmigottipati
(Plugin phase 2): Add implementation to discover and run Phase 2 Plugins
2fe045d 
Add discovery of external plugins

Define external plugin interface

Add subcommands functionality for init, create api, createa webhook, edit

Add unit tests for Discovery and Scaffold functions

Signed-off-by: Rashmi Gottipati <chowdary.grashmi@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2022
@camilamacedo86
Copy link
Member

/hold cancel

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 13, 2022
@k8s-ci-robot k8s-ci-robot merged commit 265658e into kubernetes-sigs:master May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmrodri jmrodri jmrodri requested changes

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@Adirio Adirio Awaiting requested review from Adirio

@estroz estroz Awaiting requested review from estroz

Assignees

@camilamacedo86 camilamacedo86

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rashmigottipati @k8s-ci-robot @camilamacedo86 @jmrodri @estroz