-
Notifications
You must be signed in to change notification settings - Fork 133
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
NewServiceSyncController #305
NewServiceSyncController #305
Conversation
return nil | ||
} | ||
|
||
// if err := SyncService(); err != nil { |
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.
TODO: here is where the current service
sync logic will be wired in once removed from the original sync loop.
pkg/console/operator/sync_v400.go
Outdated
@@ -68,6 +68,9 @@ func (co *consoleOperator) sync_v400(updatedOperatorConfig *operatorv1.Console, | |||
return rtErr | |||
} | |||
|
|||
// TODO: remove this from this sync loop and treat it as its own sync loop. |
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.
Extracting this next.
b17be63
to
bec9e19
Compare
bec9e19
to
85a4e37
Compare
85a4e37
to
86712ff
Compare
/retest |
86712ff
to
31970d2
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.
Couple of small comments, otherwise looks great ! 👍
31970d2
to
ea543fc
Compare
@jhadvig most items addressed & updated, 2 with comments. Thx! |
ea543fc
to
e293395
Compare
/retest
|
/retest
|
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.
Two small nist otherwise its ready for merge :) 👍
e293395
to
9035114
Compare
…m main operator control loop
9035114
to
53c6ab9
Compare
rebased. |
/retest |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benjaminapetersen, jhadvig 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest
|
@benjaminapetersen: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
/retest Please review the full test history for this PR and help us cut down flakes. |
Split SyncService() func into separate NewServiceSyncController() from main operator control loop.
Rather than one mega controller doing all the things, we should have many smaller controllers doing specific things. The first step will be to split off by resources as this is fairly simple. However, some resources, such as the Deployment, may be split into several if it makes sense.
This
controller
does not make use ofmonis.app/go/openshift/operator
boilerplate, thus it implements all of the following:which, for the most part, is pretty generic.
@jhadvig FYI, this may be helpful to look at for the CRDs control loop.