[WIP] Add event handler to Applier/Destroyer #1199
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
karlkfi
force-pushed
the
karl-applier-events
branch
from
ac9518a
to
7c70ff3
Compare
karlkfi
force-pushed
the
karl-applier-events
branch
from
9d0406a
to
54f990a
Compare
| // This method may be called while Destroy is running, to get the set of | ||
| // errors encountered so far. | ||
| Errors() status.MultiError | ||
| Apply(ctx context.Context, eventHandler func(Event), desiredResources []client.Object) (ObjectStatusMap, *stats.SyncStats) |
There was a problem hiding this comment.
The returned values are only used for logging. What about logging inside the inner functions without returning anything?
There was a problem hiding this comment.
That's what it was doing before, but returning them lets us test them.
| validationErrs status.MultiError | ||
| applyErrs status.MultiError |
There was a problem hiding this comment.
I like the change. The applyErrs now is only specific to the apply process, and doesn't not require cleaning up in the destroy time.
|
|
||
| // Return a copy to avoid persisting caller modifications | ||
| return status.Append(nil, a.errs) | ||
| func (a *supervisor) sendErrorEvent(err error, eventHandler func(Event)) { |
There was a problem hiding this comment.
The sendErrorEvent function doesn't need to be a member of the supervisor.
| // This method may be called while Destroy is running, to get the set of | ||
| // errors encountered so far. | ||
| Errors() status.MultiError | ||
| Apply(ctx context.Context, eventHandler func(Event), desiredResources []client.Object) (ObjectStatusMap, *stats.SyncStats) |
There was a problem hiding this comment.
What about renaming eventHandler to errorEventHandler or errorHandler to distinguish it from supervisorEventHandler?
There was a problem hiding this comment.
I plan on adding inventory events to it, errors are just the first pass.
But yes, having two event handlers in the same file is a bit confusing. Open to other suggestions though.
I was thinking maybe the supervisorEventHandler could be merged with the supervisor, but the supervisorEventHandler has a destroy flag I don't really want to pass to all the methods individually.
karlkfi
force-pushed
the
karl-applier-events
branch
from
54f990a
to
0a9a967
Compare
|
/retest |
- Resources' DeclaredUnstructureds, DeclaredObjects, DeclaredGVKs, and getObjectSet were previously iterating over a Go map, causing the output to be randomly ordered. This change replaces the map with an OrderedMap that can be iterated in insert order. This makes it easier to test classes that use declared.Resources, including Applier.Apply and parse.parseAndUpdate. - Add fakes for Applier, Remediator, and ConfigParser - Add test validation for Apply, Remediate, and Parse inputs and outputs - Fix fake.RootSync/RepoSync to avoid populating the status. These return a FileObject, which never have a status when created by the parser. - Fix filesystem.AsCoreObjects to return nil when empty - Replace kpt.dev/configsync/pkg/util/ordered with github.com/elliotchance/orderedmap/v2 which supports generics and actually has unit tests.
Use an injected event handler function to avoid needing to cache errors in the supervisor. Instead, cache errors in the Updater, which already has a lock for this. This event handler simplifies the Applier/Destroyer API and makes it easy to add more events in the future, like for inventory updates.
karlkfi
force-pushed
the
karl-applier-events
branch
from
0a9a967
to
0d2c829
Compare
|
@karlkfi: The following test failed, say
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. I understand the commands that are listed here. |
Use an injected event handler function to avoid needing to cache errors in the supervisor. Instead, cache errors in the Updater, which already has a lock for this.
This event handler simplifies the Applier/Destroyer API and makes it easy to add more events in the future, like for inventory updates.
Dependencies: