-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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/ansible: Adding ansible operator controller and events package #473
pkg/ansible: Adding ansible operator controller and events package #473
Conversation
13b7718
to
5230f92
Compare
} | ||
if statusEvent.Event == "" { | ||
err := errors.New("did not receive playbook_on_stats event") | ||
logrus.Error(err.Error()) |
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.
Might be redundant here if we are logging err
on return. If not then this is good.
5230f92
to
7b7d97c
Compare
7b7d97c
to
10834e5
Compare
pkg/ansible/controller/source.go
Outdated
} | ||
|
||
// NewReconcileLoop - loop for a GVK. | ||
// The reconcilation loop is need because the resync period |
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: loop is needed
} | ||
} | ||
if needsUpdate { | ||
err = r.Client.Update(context.TODO(), u) |
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.
Don't need to change this now but it might be worth looking into the Status client to see if there's a benefit to using that if we're just updating the status:
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/unstructured_client.go#L136-L151
The default manager client comes with a status writer:
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/split.go#L32
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.
Added an issue to track this, and assigned it to me.
#540
LGTM |
10834e5
to
50fc0fa
Compare
pkg/ansible/controller/controller.go
Outdated
} | ||
eventHandlers := append(options.EventHandlers, events.NewLoggingEventHandler(options.LoggingLevel)) | ||
|
||
h := &AnsibleOperatorReconciler{ |
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.
h
-> aor
or r
?
statusEvent := eventapi.StatusJobEvent{} | ||
for event := range eventChan { | ||
for _, eHandler := range r.EventHandlers { | ||
go eHandler.Handle(u, event) |
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.
dp we want to track the go rountine?
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.
offline discussion: no need to track go routines for now.
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.
^ more info
the current idea with the handlers is to take actions(logs specifically) without waiting for them to finish. These actions are not supposed to be doing work in the cluster for now and therefore can't break anything. I don't see a reason to track them.
* Adding new source for reconcilation
50fc0fa
to
8561e9f
Compare
operator-framework#566) * pkg/ansible: Adding paramconv and kubeconfig to ansible operator. (operator-framework#471) * Adding runner package. (operator-framework#472) * pkg/ansible: Adding ansible operator controller and events package (operator-framework#473) * make ansible task log outputs more readable (operator-framework#545)
No description provided.