-
Notifications
You must be signed in to change notification settings - Fork 134
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
fix build-tags, fix/update deprecated features, and fix inconsistency in path used #61
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
- The "linux" build tags were redundant as they were in a _linux file - The project already states that go1.10 is a minimum (and long obsolete, so unlikely to be used still). - Format the remaining build-tags for current go versions. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These tests were testing functionality that was only supported on Linux, but stubbed for other platforms, so move them to a linux-only file. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- NSHandle could only be used on Unix systems. Given that all other parts of the code are only implemented on Linux (not for other Unix-y platforms), I moved this file to be Linux-only. - Rename "_unspecified" to "_others", which is a common suffix for such cases. - Introduce stubs for NSHandle for non-Linux platforms. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The consts were deprecated in favor of their counterparts in golang.org/x/sys/unix. This patch makes them an alias / sets them to those values, which makes it more transparent that they're the same. Also update internal uses of the deprecated consts, as they should no longer be used, and updated the "stub" function to be deprecated as well. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The bindMountPath const was in the same group as the deprecated consts, which were deprecated as a whole. This patch moves the bindMountPath const outside of the group to make sure it's not considered deprecated. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The GetFromName() function looked for network namespaces in "/var/run", whereas DeleteNamed() and NewNamed() used the "bindMountPath" const (which points to "/run/netns"). While "/var/run" should be symlink to "/run" on most distros, this is not a guarantee, so use the same paths so that at least the code is consistently using this path. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@jeffwidman @dims @pacoxu @aboch PTAL |
jeffwidman
approved these changes
Jan 13, 2023
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.
Normally I prefer separate PR's, but here it's all super straightforward and already split at the commit level.
Nice catches, thank you!
This was referenced Jan 13, 2023
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.
I split the changes in "focussed" commits, but can open some of these as separate PRs if preferred. See individual commits for details:
remove redundant build-tag comments
make tests linux-only
These tests were testing functionality that was only supported on
Linux, but stubbed for other platforms, so move them to a linux-only
file.
move package description to a doc.go
It's common practice to use a doc.go - in this case it helps with moving more code to platform-specific files, while still keeping a general description.
fix build-tags for non-linux platforms
of the code are only implemented on Linux (not for other Unix-y platforms),
I moved this file to be Linux-only.
fix unhandled error in TestGetNewSetDelete
Just a minor nit / omission in the test
make deprecated consts an alias, and don't use internally
The consts were deprecated in favor of their counterparts in
golang.org/x/sys/unix. This patch makes them an alias / sets them
to those values, which makes it more transparent that they're the
same.
Also update internal uses of the deprecated consts, as they
should no longer be used, and updated the "stub" function
to be deprecated as well.
don't deprecate bindMountPath
The
bindMountPath
const was in the same group as the deprecated consts,which were deprecated as a whole. This patch moves the
bindMountPath
const outside of the group to make sure it's not considered deprecated.
GetFromName: use /run/netns instead of /var/run/netns
The
GetFromName()
function looked for network namespaces in "/var/run",whereas
DeleteNamed()
andNewNamed()
used thebindMountPath
const (whichpoints to "/run/netns"). While "/var/run" should be symlink to "/run" on
most distros, this is not a guarantee, so use the same paths so that at
least the code is consistently using this path.