-
Notifications
You must be signed in to change notification settings - Fork 39.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
Federation service controller #26034
Conversation
mfanjie
commented
May 22, 2016
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
2 similar comments
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
@quinton-hoole I created this PR based on #26020, when you finish code review on the wip #26034, I will update all changes on both branches, so we can be ready for code merge. |
ok to test |
Thanks @mfanjie ! |
Fixes #23848 |
Still a few minor govet and flags issues to resolve, then LGTM.
|
5f44379
to
6b589e3
Compare
resolved above problems and squashed commit. |
6b589e3
to
032e0ab
Compare
verify-govet is still failing. From logs:
|
@mfanjie govet is still not quite happy
|
Snap :-) |
acked, it is strange that I can not get these two errors when I ran govet.sh on my laptop... |
032e0ab
to
25f292d
Compare
pushed again, I will monitor the e2e result. |
s.queue.Add(key) | ||
} | ||
|
||
// Run starts a background goroutine that watches for changes to Federated services |
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.
federation services
Still trying to understand the code. Have added a few comments. |
cc @kubernetes/sig-cluster-federation |
f535db0
to
9d67f9d
Compare
@nikhiljindal all your comments addressed. |
LGTM! Needs to rebase against upstream/maser once #26020 merges (hopefully within the next few hours at most). Then ready for merge, once checks all pass. |
@mfanjie If you rebase this I should be able to merge it now. |
9d67f9d
to
15afaa7
Compare
@quinton-hoole rebased and pushed |
failed for one checking, working on it. In addition, I found some error on clustercontroller.go, even it is not related to this PR but it impacts the cluster status, so we have to manually update the cluster status to make service controller working properly, opened issue #26480 . |
089325a
to
6133db3
Compare
Thanks @mfanjie. As soon as the checks pass, I'll merge this. |
GCE e2e build/test passed for commit 089325a529d5888e119de6cd0f5f8c3aeca0cf90. |
All checks have passed. Merge queue is blocked, so manually merging, as this is a v1.3 blocker, with several other blocker PR's waiting for it to merge. |
this is a sub task of #23653 , add linkage. |
// dnsProvider is the provider for dns services. | ||
DnsProvider string `json:"dnsProvider"` | ||
// dnsConfigFile is the path to the dns provider configuration file. | ||
DnsConfigFile string `json:"ndsConfigFile"` |
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.
@quinton-hoole @mfanjie ndsConfigFile
probably should be dnsConfigFile
(still exists on master)
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.
You are right.
Being fixed in #27158
Though the json field name is not useful yet.