Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sandbox_externalkey.go: split for cross compilation #770

Merged
merged 1 commit into from
Nov 25, 2015
Merged

sandbox_externalkey.go: split for cross compilation #770

merged 1 commit into from
Nov 25, 2015

Conversation

runcom
Copy link
Member

@runcom runcom commented Nov 23, 2015

runc/libcontainer split the State struct into platform specific structs
in
opencontainers/runc@fe1cce6.
As a result, NamespacePaths isn't anymore in a global struct and
libnetwork is not cross-compiling in Docker (specifically on Windows) because
sandbox_externalkey.go is using NamespacePaths.
This patch splits sandbox_externalkey.go into platform specific
files and moves common things to a generic sandbox_externalkey.go.

related to moby/moby#18106

Signed-off-by: Antonio Murdaca runcom@redhat.com

@runcom runcom mentioned this pull request Nov 23, 2015
Merged
@runcom runcom force-pushed the fix-cross-compile branch 2 times, most recently from dd4b60b to 2ac51e8 Compare November 23, 2015 23:31
@runcom
Copy link
Member Author

runcom commented Nov 24, 2015

ping @mavenugo @mrjana @aboch (sorry for the noise)

mrjana
mrjana reviewed Nov 24, 2015
View reviewed changes
sandbox_externalkey_unsupported.go

// SetExternalKey provides a convenient way to set an External key to a sandbox
func SetExternalKey(controllerID string, containerID string, key string) error {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't all these return an appropriate error? We specifically have an error interface defined for this purpose.

https://github.com/docker/libnetwork/blob/master/types/types.go#L480

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update, wasn't aware of that kind of error, thanks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrjana there's a problem with https://github.com/docker/libnetwork/blob/master/controller.go#L200 because if we now return an error something can break if some Docker code path come across this and controller.go seems pretty platform agnostic, WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made what's used outside those files to return nil as a no-op and return a ErrNotImplemented otherwise, I hope it makes sense now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think only SetExternalKey should return that kind of error because it's part of the API, the other functions can basically return nil

@runcom runcom force-pushed the fix-cross-compile branch 3 times, most recently from 03f1c3c to 41eff70 Compare November 24, 2015 01:23
@dave-tucker dave-tucker mentioned this pull request Nov 24, 2015
Merged
@jessfraz jessfraz mentioned this pull request Nov 24, 2015
Merged
@mrjana
Copy link
Contributor

mrjana commented Nov 24, 2015

LGTM

@mavenugo
Copy link
Contributor

@runcom this fix seems to have made the externalkey support specific to linux.
Are we sure this doesn't have a negative impact on FreeBSD/ARM, etc ?
Can you please confirm ?

@mavenugo mavenugo mentioned this pull request Nov 25, 2015
Merged
@runcom runcom force-pushed the fix-cross-compile branch 3 times, most recently from dfda5e0 to 3b00cdf Compare November 25, 2015 13:42
jessfraz
jessfraz reviewed Nov 25, 2015
View reviewed changes
sandbox_externalkey_unix.go
@@ -0,0 +1,177 @@
// +build linux freebsd
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe !windows instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright

@runcom
Copy link
Member Author

runcom commented Nov 25, 2015

@jfrazelle updated as per your comment

@jessfraz
Copy link

\o/ all up to @mavenugo

@mavenugo
Copy link
Contributor

Sorry about the delay.
With the changes to !windows shouldn't we just add a _windows.go file and not do unsupported? Even if we do _unsupported.go, isn't it appropriate to include just the Windows target ?

@runcom
Copy link
Member Author

runcom commented Nov 25, 2015

make sense, most docker code is the way you said @mavenugo (i.e. https://github.com/docker/docker/blob/master/daemon/container_windows.go and https://github.com/docker/docker/blob/master/daemon/container_unix.go)
I'll update the PR (another example is https://github.com/docker/docker/tree/master/pkg/idtools)

@runcom
sandbox_externalkey.go: split for cross compilation
806157b 
runc/libcontainer split the `State` struct into platform specific structs
in
opencontainers/runc@fe1cce6.
As a result, `NamespacePaths` isn't anymore in a global struct and
libnetwork is not cross-compiling in Docker (specifically on Windows) because
`sandbox_externalkey.go` is using `NamespacePaths`.
This patch splits `sandbox_externalkey.go` into platform specific
files and moves common things to a generic `sandbox_externalkey.go`.

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@runcom
Copy link
Member Author

runcom commented Nov 25, 2015

green again :) I'm testing with Docker&libcontainer in make cross

@mavenugo
Copy link
Contributor

Thanks. LGTM

@aboch
Copy link
Contributor

aboch commented Nov 25, 2015

LGTM

aboch added a commit that referenced this pull request Nov 25, 2015
@aboch
Merge pull request #770 from runcom/fix-cross-compile
04cc1fa 
sandbox_externalkey.go: split for cross compilation
@aboch aboch merged commit 04cc1fa into moby:master Nov 25, 2015
@runcom runcom deleted the fix-cross-compile branch November 25, 2015 23:40
@runcom
Copy link
Member Author

runcom commented Nov 25, 2015

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@runcom @mrjana @mavenugo @jessfraz @aboch