Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upkube-aggregator: split openapi spec aggregator from controller code #73953
Conversation
This comment has been minimized.
This comment has been minimized.
|
@sttts: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
k8s-ci-robot
added
size/L
do-not-merge/release-note-label-needed
cncf-cla: yes
needs-kind
needs-sig
needs-priority
labels
Feb 12, 2019
sttts
added
priority/important-soon
release-note-none
and removed
do-not-merge/release-note-label-needed
labels
Feb 12, 2019
k8s-ci-robot
removed
the
needs-priority
label
Feb 12, 2019
sttts
added
the
sig/api-machinery
label
Feb 12, 2019
k8s-ci-robot
removed
the
needs-sig
label
Feb 12, 2019
k8s-ci-robot
requested review from
caesarxuchao
and
rramkumar1
Feb 12, 2019
sttts
added
the
kind/cleanup
label
Feb 12, 2019
k8s-ci-robot
added
sig/network
and removed
needs-kind
labels
Feb 12, 2019
sttts
force-pushed the
sttts:sttts-simplify-kube-aggregator-openapi
branch
2 times, most recently
from
6502bf5
to
5d288ea
Feb 12, 2019
sttts
added some commits
Feb 12, 2019
sttts
force-pushed the
sttts:sttts-simplify-kube-aggregator-openapi
branch
from
5d288ea
to
f132254
Feb 12, 2019
mfojtik
reviewed
Feb 12, 2019
| var _ SpecAggregator = &specAggregator{} | ||
|
|
||
| // This function is not thread safe as it only being called on startup. | ||
| func (s *specAggregator) addLocalSpec(spec *spec.Swagger, localHandler http.Handler, name, etag string) { |
This comment has been minimized.
This comment has been minimized.
mfojtik
Feb 12, 2019
Contributor
can we add s. addLocalSpecCalled = true and then panic when this function is called again?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
sttts
Feb 12, 2019
Author
Contributor
Why?
It's only called from the speAggregator constructor BuildAndRegisterAggregator. What are you fearing here?
This comment has been minimized.
This comment has been minimized.
mfojtik
Feb 12, 2019
Contributor
@sttts fearing every function that has "should be only called once on startup" :-)
This comment has been minimized.
This comment has been minimized.
mbohlool
Feb 12, 2019
Member
I will point out that the function is private and restricted to be used in this file. For that I won't add a once to be called logic. Also it may run more than once for local specs on startup.
sttts
force-pushed the
sttts:sttts-simplify-kube-aggregator-openapi
branch
from
f132254
to
a48c9f2
Feb 12, 2019
This comment has been minimized.
This comment has been minimized.
sttts
assigned
mbohlool and
deads2k
Feb 12, 2019
This comment has been minimized.
This comment has been minimized.
|
I only see refactoring and moving codes around. The new structure make sense to me. Thanks for doing this. /lgtm |
k8s-ci-robot
added
the
lgtm
label
Feb 12, 2019
This comment has been minimized.
This comment has been minimized.
|
/approve |
This comment has been minimized.
This comment has been minimized.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, sttts 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 |
sttts commentedFeb 12, 2019
•
edited
Untangle the actual merge logic and the aggregator openapi spec state from the controller logic. Simplifies the code.