Conversation
There was a problem hiding this comment.
Now that we are introducing sandbox as the top-level resource, Do we need to expose this backend resource any more ?
There was a problem hiding this comment.
We still need a URL for the Join/Leave in the POST/DELETE list.
If we do so, then I feel we should provide the GET for the same URL.
There was a problem hiding this comment.
The REST resources needs a cleanup. Not necessarily in this PR, but in a later PR once the UX is in place.
There was a problem hiding this comment.
Yes, once the UX story is finalized, we can remove the URL endpoints which no longer apply.
|
@aboch Thanks for handling all the comments. LGTM. |
api/api.go
Outdated
There was a problem hiding this comment.
Can you please convert this into a switch/case statement? Go lang switch/case is feature rich and it's always preferred for readability purposes.
There was a problem hiding this comment.
Actually you can create a global map of function type and just retrieve the appropriate function from that. That is more optimal than instantiating the functions every call
There was a problem hiding this comment.
Thanks, will define the functions somewhere else and only once.
There was a problem hiding this comment.
I am not following the logic of having this byID argument while there is only one way to retrieve the sandboxes. Why do we need this?
There was a problem hiding this comment.
It's just to maintain the same construct I have for network and endpoint.
So that all the GET serving function look alike and follow the same steps when looking for the resource.
endpoint.go
Outdated
There was a problem hiding this comment.
What I meant was this should completely become an option passed while creating a sandbox, isn't it? Because this should happen once during sandbox create and there should be no way to add them for every endpoint.
There was a problem hiding this comment.
Right. Let me fix that. Thanks.
There was a problem hiding this comment.
Actually here too, buildHostsFiles need to know to which network we are connecting the sandbox so that it retrieves the SrvcRecords from it and need to know the endpoint to get the IP of the interface.
Seems like this operation cannot be completely moved to sandbox creation.
|
@mrjana I pushed a new change based on your comments. |
There was a problem hiding this comment.
Here it looks we were not failing the endpoint join if we fails to update the endpoint into the datastore (err not visible outside of if scope).
New changes took care of this.
- Maps 1 to 1 with container's networking stack
- It holds container's specific nw options which
before were incorrectly owned by Endpoint.
- Sandbox creation no longer coupled with Endpoint Join,
sandbox and endpoint have now separate lifecycle.
- LeaveAll naturally replaced by Sandbox.Delete
- some pkg and file renaming in order to have clear
mapping between structure name and entity ("sandbox")
- Revisited hosts and resolv.conf handling
- Removed from JoinInfo interface capability of setting hosts and resolv.conf paths
- Changed etchosts.Build() to first write the search domains and then the nameservers
Signed-off-by: Alessandro Boch <aboch@docker.com>
|
@aboch Thanks for being patient and taking care of all the comments and doing a bit more cleanup on your own. LGTM |
4 participants
before were incorrectly owned by Endpoint.
sandbox and endpoint have now separate lifecycle.
mapping between externally exposed resources
and internal structures
This PR addresses point 1, 2 and first part of point 3 in issue #429
Signed-off-by: Alessandro Boch aboch@docker.com