-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Implement Simple Pipeline Controller with tests #75
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tejal29 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Just noticed, Bulk of changes are in vendor directory. |
2987370
to
53f4cde
Compare
1eae748
to
cbf094e
Compare
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.
Niiiiiice! Very cool!
I think you've done a lot of work here that is going to be super valuable for everyone implementing controllers going forward! My biggest piece of feedback is that if I was to have to do the same thing (e.g. for a different controller/type), looking at the tests that you've added for guidance, I'd be a bit confused about what I needed to do, so it would be great to have some kind of docs or guidance for future unit test authors.
pkg/constants/constants.go
Outdated
|
||
const ( | ||
//PipelineRunAgent is the logging agent for pipeline run controller | ||
PipelineRunAgent = "pipelinerun-controller" |
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.
what is an agent in this context? is that a kubernetes term?
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.
This is the logging agent. The Log line will dump logs with this agent name.
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.
I'm just wondering what "agent" means specifically, is it a kubernetes thing or golang thing?
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.
nope. this more of logging thing. Its used when logging.
I can rename this to loggerName if that helps.
var ( | ||
runsListerFunc = runsLister | ||
pipelinesListerFunc = pipelinesLister | ||
getRecordFunc = getRecorder |
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.
This could also done with one NewController
that takes functional arguments (à-la-builder in a way). That would allow to setup sane defaults, easily overridable (and future-proof).
type ControllerConfig {
lister func // …
// …
}
func Lister(f func …) func(*ControllerConfig) {
return func(c *ControllerConfig) {
c.lister = f
}
}
// Any non-required function can have a sane default and thus be written as a "function operator"
func NewController(
k kubernetes.Interface,
c clientset.Interface,
l *zap.SugaredLogger,
ops ...func(*ControllerConfig)
) controller.Interface {
c := &ControllerConfig{
// … with sane defaults
}
for _, op := range ops {
op(c)
}
// […]
This pipeline controller records events when a pipelinerun points to right pipeline reference. This implementation, for now only records events if the pipelinerun object points to valid pipeline.
Add unit tests for new reconcilers. Unit tests, uses fake kuberneters go client and REST client. They run Reconciler and obsevers errors logged. The tests, then verifies the logged errors.
cbf094e
to
fd08fae
Compare
Added documentation on adding tests for controller.
fd08fae
to
e46ec55
Compare
@bobcatfish , @pivotal-nader-ziada and @vdemeester This is ready for review with reconciler and documentations on how to test controller. |
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.
A good reconciler start, I think it would be good to merge this in and then add the functionality of creating pipelineRun and taskRun along with more tests
@tejal29
small changes we can hopefully change before merging this in:
- Fix the
Gopkg.lock
diff so it doesn't cause conflicts later - Add the newController in
cmd/controller/main.go
t.Errorf("expected list to be called, found %s", action.GetVerb()) | ||
} | ||
``` | ||
|
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.
neat!
5298a69
to
94c3d36
Compare
94c3d36
to
6073042
Compare
Thanks @tejal29 /lgtm |
Addresses 1 pat of #61
This PR, implements simple pipeline controller which records events when a pipelinerun points to right pipeline reference.
This implementation, for now does nothing and logs errors if there the pipeline run refers to invalid ref.