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

Update linux plugins in doc #925

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

astraw99
Copy link

@astraw99 astraw99 commented Jul 8, 2023

To make the doc better.

@astraw99 astraw99 force-pushed the patch-1 branch 2 times, most recently from 5da3a04 to 76f6932 Compare July 8, 2023 12:53
@astraw99 astraw99 changed the title Add the tap and sbr plugins in doc Update linux plugins in doc Jul 8, 2023
@@ -1,14 +1,18 @@
plugins/ipam/dhcp
plugins/ipam/host-local
Copy link
Member

Choose a reason for hiding this comment

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

I think these plugins work on windows, so they should not be in this list.

Copy link
Author

Choose a reason for hiding this comment

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

But there are plugins like plugins/ipam/host-local work both on linux and windows.
What about changing the file name from linux_only.txt to linux.txt, and windows_only.txt to windows.txt?
Then add the plugins into the list accordingly, it will be more clarify.

Copy link
Member

Choose a reason for hiding this comment

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

if you'd like, feel free. But right now this PR is removing test coverage it shouldn't.

Copy link
Author

Choose a reason for hiding this comment

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

Updated with filename changed.
PTAL thanks.

Copy link
Member

Choose a reason for hiding this comment

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

where is this file used? If it is just for documentation purposes, wouldn't it be better to say all except the windows folder?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, currently the linux.txt file is just for doc purposes.
Updated, PTAL thanks.

Copy link
Member

Choose a reason for hiding this comment

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

but now you are removing host-local from windows which is not ok

Copy link
Author

Choose a reason for hiding this comment

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

As host-local is both working in linux and windows, changed the filename from linux_only.txt to linux.txt, and windows_only.txt to windows.txt.
PTAL thanks.

Signed-off-by: astraw99 <wangchengiscool@gmail.com>
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

3 participants