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.

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

pkg/sdk: combining all pkgs under pkg/sdk into a single pkg #242

Merged
merged 1 commit into from
May 15, 2018

Conversation

rithujohn191
Copy link
Contributor

Addresing #162. This PR combines the pkg/sdk/{action,handler,informer,query,types} into one package called sdk.

The new structure:

go doc "github.com/operator-framework/operator-sdk/pkg/sdk"
package sdk // import "github.com/operator-framework/operator-sdk/pkg/sdk"

func Create(object Object) (err error)
func Delete(object Object, opts ...DeleteOption) (err error)
func Get(into Object, opts ...GetOption) error
func Handle(handler Handler)
func List(namespace string, into Object, opts ...ListOption) error
func Run(ctx context.Context)
func Update(object Object) (err error)
func Watch(apiVersion, kind, namespace string, resyncPeriod int)
type Context struct{ ... }
type DeleteOp struct{ ... }
    func NewDeleteOp() *DeleteOp
type DeleteOption func(*DeleteOp)
    func WithDeleteOptions(metaDeleteOptions *metav1.DeleteOptions) DeleteOption
type Event struct{ ... }
type GetOp struct{ ... }
    func NewGetOp() *GetOp
type GetOption func(*GetOp)
    func WithGetOptions(metaGetOptions *metav1.GetOptions) GetOption
type Handler interface{ ... }
    var RegisteredHandler Handler
type Informer interface{ ... }
    func NewInformer(resourcePluralName, namespace string, resourceClient dynamic.ResourceInterface, ...) Informer
type ListOp struct{ ... }
    func NewListOp() *ListOp
type ListOption func(*ListOp)
    func WithListOptions(metaListOptions *metav1.ListOptions) ListOption
type Object runtime.Object

@rithujohn191
Copy link
Contributor Author

@hasbro17 @ericchiang we might want to think about how we can further improve on this design structure and break it up its more logical modules.

@etiennecoutaud
Copy link
Contributor

I didn't check your travis logs but if your test seems to be ok, check your spaces/tabs in the test string template, I had the same problem.

@rithujohn191
Copy link
Contributor Author

All travis tests pass now 👍 Needed a small change in pkg/generator/handler_tmpl.go

@hasbro17
Copy link
Contributor

Testing this PR out manually with the user-guide example since we don't have an e2e test in place right now.

@ericchiang
Copy link

we might want to think about how we can further improve on this design structure and break it up its more logical modules.

+1

Yeah, definitely "Optimize for Godoc".

I think #169 and #161 will help here. Would also be interesting to know why we need both DeleteOp and DeleteOption, and why thetype Object runtime.Object type declaration exists.

PR lgtm though @hasbro17 should approve.

@hasbro17
Copy link
Contributor

I've tested out the README and user-guide and the changes seem fine.

@rithujohn191 Can you also please update the CHANGELOG as well. Just mention that the action and query pkgs have been consolidated into pkg/sdk, and so their use in the handler is now sdk.Create(), sdk.Get() etc.

#169 is the next issue we're working on to define all the underlying pieces of the core SDK. Removing all the global state and defining clear structures will hopefully make the design more clear.

Would also be interesting to know why we need both DeleteOp and DeleteOption

@ericchiang DeleteOp is the wrapper around all the possible options(currently only metav1.DeleteOptions{}) and DeleteOption is the functional option type.
https://github.com/tmrts/go-patterns/blob/master/idiom/functional-options.md#options
https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

why the type Object runtime.Object type declaration exists

I think in the original design we were anticipating to control all types in the SDK APIs but over time I'm not sure of the benefits of that. We're going to revisit this and #161 in the upcoming restructuring.

@rithujohn191
Copy link
Contributor Author

@fanminshi
Copy link
Contributor

lgtm thanks!

@hasbro17
Copy link
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants