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.

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
merged 8 commits into from
Jan 13, 2023

Conversation

thaJeztah
Copy link
Contributor

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

  • 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.

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

  • 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.

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() 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.

- 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>
@thaJeztah
Copy link
Contributor Author

@jeffwidman @dims @pacoxu @aboch PTAL

Copy link
Collaborator

@jeffwidman jeffwidman left a 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!

@jeffwidman jeffwidman merged commit a21f45a into vishvananda:master Jan 13, 2023
@thaJeztah thaJeztah deleted the update_deprecated branch January 13, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants