-
Notifications
You must be signed in to change notification settings - Fork 107
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
docs: network_connections module is only meant for internal usage #686
Conversation
README.md
Outdated
@@ -319,6 +319,18 @@ role. | |||
Similar to `controller` and `vlan`, the `parent` references the connection profile in | |||
the ansible role. | |||
|
|||
#### Network_connections Module |
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.
No need to add so much text about unsupported module. In my opinion, it is better to not document unsupported modules at all. Usually, things that are not documented are considered unsupported.
Module has info that it is not supported to be run outside the role in https://github.com/linux-system-roles/network/blob/main/library/network_connections.py#L17
And I think it's enough
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.
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.
@tyll would you say that we need a note about module being unsupported in README when the module has a warning in it?
I'd say that not mentioning this module in README at all is enough. And if we want to be extra clear, a one sentence warning in README would be enough, no need for a separate section.
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.
yes, I agree that a single sentence is better. The extra sections sound to me like they are generated from an AI and do not provide significant information.
README.md
Outdated
@@ -1380,6 +1399,39 @@ may be | |||
Another option maybe be to initially auto-configure the host during installation (ISO | |||
based, kickstart, etc.), so that the host is connected to a management LAN or VLAN. It | |||
strongly depends on your environment. | |||
#### Installation and Usage Guide |
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.
Let's not include basic instructions on how to use Ansible into separate roles. I'd expect users to go through Ansible docs prior to getting to the role.
We ship roles in collection in https://galaxy.ansible.com/ui/repo/published/fedora/linux_system_roles/
Yes, it is possible to install the role by cloning git repo, but the proper way is to get the collection.
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.
@spetrosi sorry i do not understand so what was expected from the solution(the issue fix)?
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.
Is there some other issue that requests this? This is not part of #292
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.
And we provide starting info in https://github.com/linux-system-roles/network/blob/main/README-ansible.md
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.
@spetrosi @tyll @liangwen12year so what was the these issues were asking am confused isn't it asking me to write note on the ReadMe.md? |
If I am not mistaken, you are supposed to write it in README.md. |
Can you rebase the current PR, |
ping - please rebase |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #686 +/- ##
===========================================
+ Coverage 20.50% 43.22% +22.72%
===========================================
Files 10 12 +2
Lines 1478 3100 +1622
Branches 433 0 -433
===========================================
+ Hits 303 1340 +1037
- Misses 1174 1760 +586
+ Partials 1 0 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…the role Signed-off-by: maritu <[email protected]>
0ba8dd7
into
linux-system-roles:main
Enhancement:Creating Document for the network_connections module
Reason:To guide users and developers to use the network_connections module only for internal usage
Result:with the adding of this documentation user and developers will understand No Direct Invocation for network_connections module,they should refrain from directly invoking the network_connections module outside of its designated role.
Issue Tracker Tickets (Jira or BZ if any):#292