Skip to content
This repository has been archived by the owner on Apr 8, 2022. It is now read-only.

add handler for exec local command #116

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

missedone
Copy link
Contributor

No description provided.

handleEvent(s, newObj, "updated")
}

func handleEvent(s *Exec, obj interface{}, action string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add handler for running local command when receiving events

@jbianquetti-nami
Copy link
Contributor

Hi @missedone.
Looks nice. Would you provide some content to the README file explaining the use case and an example of your commit? I think a useful README it's the first thing to check in case of questions.

@jbianquetti-nami jbianquetti-nami self-requested a review June 12, 2018 16:43
Copy link
Contributor

@jbianquetti-nami jbianquetti-nami left a comment

Choose a reason for hiding this comment

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

Would you provide some content to the README file explaining the use case and an example of your commit?

Looks like you're also adding support for configmaps, right? That seems to be managed by another PR, so it's better to decouple PRs between them.

Thanks for your time!

@missedone
Copy link
Contributor Author

sure, will add example soon

yes, configmap PR goes #114 , and namespace filter PR goes #115

@missedone
Copy link
Contributor Author

@jbianquetti-nami , example added: 369bdcf

@missedone
Copy link
Contributor Author

just rebased the PR, please review again, thx

@jbianquetti-nami
Copy link
Contributor

I think it's a nice feature to have but needs some care. Can you fix conflicts?

@missedone
Copy link
Contributor Author

sure, will resolve the conflicts.

# Conflicts:
#	README.md
#	cmd/config.go
#	config/config.go
#	pkg/client/run.go
#	pkg/handlers/handler.go
@missedone
Copy link
Contributor Author

conflicts resolved

Copy link
Contributor

@jbianquetti-nami jbianquetti-nami left a comment

Choose a reason for hiding this comment

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

For me, it's OK from the operational POV.
IDK if someone would raise some concerns about security @jjo @mkmik

limitations under the License.
*/

package exec
Copy link

@mkmik mkmik Jun 24, 2019

Choose a reason for hiding this comment

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

Putting my end user's hat on, I'd love to read some more documentation about what problems does the handler implemented in this package address and some instructions about how to use it.

(A package level comment seems to be a good place to put such doc)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

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

Successfully merging this pull request may close these issues.

3 participants