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

feat(net): add option -x to unshare network #130

Merged
merged 11 commits into from
Oct 31, 2023
Merged

feat(net): add option -x to unshare network #130

merged 11 commits into from
Oct 31, 2023

Conversation

ezrizhu
Copy link
Collaborator

@ezrizhu ezrizhu commented Oct 26, 2023 •
edited
Loading

This PR adds an option -x to unshare the network by adding --net to the first unshare invocation.

  • add option to try
  • add to unit test
eric@ide0:~/try$ ./try ip --brief link
lo               UNKNOWN        00:00:00:00:00:00 <LOOPBACK,UP,LOWER_UP>
ens18            UP             ce:b8:f6:2a:67:af <BROADCAST,MULTICAST,UP,LOWER_UP>
eric@ide0:~/try$ ./try -x ip --brief link
lo               DOWN           00:00:00:00:00:00 <LOOPBACK>

Note: I would use ping -c1 1.1 instead of curl 1.1 but #131

Closes #127

@ezrizhu ezrizhu requested a review from mgree October 26, 2023 06:10
@ezrizhu ezrizhu self-assigned this Oct 26, 2023
@ezrizhu ezrizhu marked this pull request as ready for review October 26, 2023 06:20
angelhof
angelhof reviewed Oct 27, 2023
View reviewed changes
try Outdated Show resolved Hide resolved
angelhof
angelhof reviewed Oct 27, 2023
View reviewed changes
Copy link
Member

@angelhof angelhof left a comment

Choose a reason for hiding this comment

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

The changes look good to me! Don't we need to also modify manpages and completions?

@ezrizhu
Copy link
Collaborator Author

ezrizhu commented Oct 27, 2023 •
edited
Loading

Don't we need to also modify manpages and completions?

Oops I forgot about that, they have been pushed.

@ezrizhu ezrizhu requested a review from angelhof October 27, 2023 21:49
angelhof
angelhof approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@angelhof angelhof left a comment

Choose a reason for hiding this comment

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

Looks good to me! Are the failing tests something to worry about?

@ezrizhu
Copy link
Collaborator Author

ezrizhu commented Oct 30, 2023 •
edited
Loading

Are the failing tests something to worry about?

Likely not! They’re failing due to #128

gliargovas
gliargovas approved these changes Oct 30, 2023
View reviewed changes
Copy link
Collaborator

@gliargovas gliargovas left a comment

Choose a reason for hiding this comment

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

LGTM! Nicely done @ericzty

mgree
mgree requested changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few small tweaks.

docs/try.1.md Outdated Show resolved Hide resolved
test/network.sh Outdated
# Test if network works normally
"$TRY" curl 1.1 || return 1

# Test if ping fails when network is unshared
Copy link
Contributor

Choose a reason for hiding this comment

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

What is curl 1.1 supposed to do? You say ping but run curl. Maybe use ping 8.8.8.8?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

curl 1.1 expands to curl 1.0.0.1, which gets you the frontpage of cloudflare's public dns resolver, ping doesn't work due to #131

test/network.sh Outdated Show resolved Hide resolved
try Outdated Show resolved Hide resolved
try Outdated Show resolved Hide resolved
@ezrizhu ezrizhu requested a review from mgree October 31, 2023 05:01
mgree
mgree approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

LGTM! (Made two tiny text tweaks.)

@ezrizhu ezrizhu merged commit b976c39 into main Oct 31, 2023
10 of 17 checks passed
@ezrizhu ezrizhu deleted the eric-unshare-net branch October 31, 2023 18:23
mgree added a commit that referenced this pull request Oct 31, 2023
@mgree
turn off shellcheck warning
e9afc9b 
missed in merge of #130
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gliargovas gliargovas gliargovas approved these changes

@angelhof angelhof angelhof approved these changes

@mgree mgree mgree approved these changes

Assignees

@ezrizhu ezrizhu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Flag to disable network access
4 participants
@ezrizhu @mgree @angelhof @gliargovas