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

Introducing PodStatusHooks #78

Merged
merged 10 commits into from
Oct 7, 2019
Merged

Introducing PodStatusHooks #78

merged 10 commits into from
Oct 7, 2019

Conversation

mcmadhan01
Copy link

@mcmadhan01 mcmadhan01 changed the title InPodStatusHooks Introduding PodStatusHooks Sep 26, 2019
@mcmadhan01 mcmadhan01 changed the title Introduding PodStatusHooks Introducing PodStatusHooks Sep 26, 2019
viprgupta and others added 6 commits September 26, 2019 20:02
 Allow hooks extensibility by pushing pod/task status to a channel

fix config key used to fetch configured hooks

move general, example plugins to pluginimpl directory

bug fixes and unit tests

fix go imports
plugin/type.go Outdated
Execute(podStatus string, data interface{}) error
// BestEffort is invoked in case a Execute returned an error and is expected to return a bool to indicate
// if the execution needs to continue with the next available hook or not
BestEffort() bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. Need this to determine whether execution need to proceed to next available hook. It would be better to pass this data in Execute( ) response such as (bool, error). Combining BestEffort() with Execute, as well would make it flexible to handle various situations -- some may be best effort while others could be not.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, makes sense to combine this or introduce a custom error type. I will make that change.

Copy link
Collaborator

@kkrishna kkrishna left a comment

Choose a reason for hiding this comment

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

LGTM

@vipragupta vipragupta merged commit 633eea1 into paypal:develop Oct 7, 2019
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.

3 participants