Add path filtering to build session client #33859
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tonistiigi
force-pushed
the
session-includepaths
branch
2 times, most recently
from
f10c179
to
3add307
Compare
thaJeztah
approved these changes
Jun 28, 2017
builder/dockerfile/builder.go
Outdated
| @@ -141,7 +141,7 @@ func (bm *BuildManager) initializeClientSession(ctx context.Context, cancel func | |||
| if options.RemoteContext == remotecontext.ClientSessionRemote { | |||
| st := time.Now() | |||
| csi, err := NewClientSessionSourceIdentifier(ctx, bm.sg, | |||
| options.SessionID, []string{"/"}) | |||
| options.SessionID) | |||
There was a problem hiding this comment.
nit: this can probably be on a single line now
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
tonistiigi
force-pushed
the
session-includepaths
branch
from
3add307
to
4141d8f
Compare
boaz0
reviewed
Jul 3, 2017
|
|
||
| // HandleConn handles an incoming raw connection | ||
| func (sm *Manager) HandleConn(ctx context.Context, conn net.Conn, opts map[string][]string) error { | ||
| sm.mu.Lock() |
There was a problem hiding this comment.
I think sm's mu is being locked twice w/o unlocking in between:
One in line 56 and second one here.
There was a problem hiding this comment.
@ripcurld0 HandleHTTPRequest does not call this function but directly the one that assumes lock has already been taken.
There was a problem hiding this comment.
Yes sorry, my bad. I didn't notice I am mixing between handleConn and HandleConn.
tiborvass
reviewed
Jul 4, 2017
|
|
||
| dialer := session.Dialer(testutil.TestStream(testutil.Handler(m.HandleConn))) | ||
|
|
||
| g, ctx := errgroup.WithContext(context.Background()) |
|
LGTM |
|
@tonistiigi test timed out on windows, maybe it's related ? |
|
@vieux I can't see how it could be as it doesn't do anything with build. For the reference: |
|
All green, merging |
This was referenced Oct 2, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR enables path filtering capabilities to the file send stream so that it can be used in the future for the builder to only ask the paths that are actually needed or for transferring individual files(for example Dockerfile). This was described as follow-up in #32677 as optional but it occurred to me that if a client doesn't have support for this then future daemons would need to detect if clients have support for this or not(possible but harder to maintain). The
fsutilpackage already had support for this but it was not enabled insession/filesync.As there isn't a daemon feature that uses it yet I added a test helper that can be used to test the full lifecycle of the session in another commit. Hopefully this will become useful in the future for testing other similar features as well.
@thaJeztah @tiborvass