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

Add support for removing a network by id #1581

Merged
merged 3 commits into from
Dec 4, 2022
Merged

Add support for removing a network by id #1581

merged 3 commits into from
Dec 4, 2022

Conversation

Mihai22125
Copy link
Contributor

Resolves #1568

First time contributor, so any feedback would be more than welcomed!

@Mihai22125 Mihai22125 force-pushed the remove_net_by_id branch 3 times, most recently from c79a006 to b153479 Compare December 1, 2022 11:37
@Zheaoli
Copy link
Member

Zheaoli commented Dec 1, 2022

And maybe we need extra docs change here to let people know they can delete network by id

@Zheaoli Zheaoli modified the milestone: v1.1.0 Dec 1, 2022
djdongjin
djdongjin reviewed Dec 1, 2022
View reviewed changes
pkg/netutil/networkwalker.go Outdated
Req string // The raw request string. name, short ID, or long ID.
MatchIndex int // Begins with 0, up to MatchCount - 1.
MatchCount int // 1 on exact match. > 1 on ambiguous match. Never be <= 0.
UniqueImages int // Number of unique images in all found images.
Copy link
Member

Choose a reason for hiding this comment

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

You can remove UniqueImages since it's not used and not relevant to network.

@Mihai22125
Copy link
Contributor Author

Thanks for the review! I will address all comments soon.

@Mihai22125 Mihai22125 requested review from Zheaoli and djdongjin and removed request for Zheaoli December 1, 2022 16:45
djdongjin
djdongjin reviewed Dec 1, 2022
View reviewed changes
Copy link
Member

@djdongjin djdongjin left a comment

Choose a reason for hiding this comment

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

You may squash into 1 commit and sign.

CI will fail if you have any non-signed commits (394e817)

Zheaoli
Zheaoli approved these changes Dec 1, 2022
View reviewed changes
Copy link
Member

@Zheaoli Zheaoli left a comment

Choose a reason for hiding this comment

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

LGTM on CI green

AkihiroSuda
AkihiroSuda reviewed Dec 2, 2022
View reviewed changes
pkg/netutil/networkwalker.go Outdated
@@ -0,0 +1,74 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in pkg/idutil/netwalker for consistency with other objects?
https://github.com/containerd/nerdctl/tree/main/pkg/idutil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @AkihiroSuda, I was thinking about this too, but networkConfig is not exported outside of netutil package, so the only solution I could figure out was to implement the walker in the same package as the struct.

Copy link
Member

Choose a reason for hiding this comment

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

You can make the networkConfig as a public struct.

I agree with @AkihiroSuda; all of the walkers could be put together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AkihiroSuda @Zheaoli Addressed, please review

@Mihai22125
Add the option to remove a network by id
98054c2 
Signed-off-by: Mihai Timbota-Belin <mihai.mtb10@gmail.com>
@Mihai22125
Export NetworkConfig struct
9165474 
Signed-off-by: Mihai Timbota-Belin <mihai.mtb10@gmail.com>
@Mihai22125
Moved NetworkWalker to another package
01ca115 
Signed-off-by: Mihai Timbota-Belin <mihai.mtb10@gmail.com>
@AkihiroSuda AkihiroSuda added this to the v1.1.0 milestone Dec 4, 2022
AkihiroSuda
AkihiroSuda approved these changes Dec 4, 2022
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit b2302b1 into containerd:main Dec 4, 2022
@aznashwan aznashwan mentioned this pull request Dec 5, 2022
Merged
@yzxiu yzxiu mentioned this pull request Dec 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djdongjin djdongjin djdongjin left review comments

@Zheaoli Zheaoli Zheaoli approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.

cannot delete network by ID
4 participants
@Mihai22125 @Zheaoli @AkihiroSuda @djdongjin