On Thu, Oct 1, 2020 at 1:19 PM Jan Tluka <jtluka@redhat.com> wrote:
Wed, Sep 30, 2020 at 04:14:52PM CEST, jurbanov@redhat.com wrote:
>From: Jozef Urbanovsky <jurbanov@redhat.com>
>
>Signed-off-by: Jozef Urbanovsky <jurbanov@redhat.com>
>---
> lnst/Recipes/ENRT/IpsecEspAeadRecipe.py | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
>diff --git a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
>index 015db74..30da2fb 100644
>--- a/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
>+++ b/lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
>@@ -57,8 +57,9 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
>
>     def test_wide_configuration(self):
>         """
>-        Test wide configuration for this recipe involves just adding an IPv4 and
>-        IPv6 address to the matched eth0 nics on both hosts and route between them.
>+        Test wide configuration for this recipe involves adding an IPv4 and
>+        IPv6 address to the matched eth0 nics on both hosts and creating a route
>+        between them.
>
>         host1.eth0 = 192.168.101.1/24 and fc00::1/64
>
>@@ -120,6 +121,11 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
>         """
>         Test wide configuration is extended with subconfiguration containing
>         IPsec tunnel with predefined parameters for both IP versions.
>+        Required variables for the configuration of IPsec tunnel are generated
>+        and yieled for later application of subconfiguration.
>+
>+        Yielded as::
>+            (new_config)

I'm not sure how much value the user gets from that.

I recommend following reading: https://www.writethedocs.org/guide/writing/beginners-guide-to-docs/#what-to-write

I think that info about yielding is quite redundant as this is obvious
from the code itself.

I'd really focus on the user point of view here, rather than from a
developer point of view.

In that case the main focus of documentation here should be to explain
__why__ this method is different from the one used in other recipes (where
it's inherited from the BaseSubConfigMixin).

Also, I think that you should mention that for this specific recipe the
sub configurations are combinations of IPSec mode (transport, tunnel)
and encryption algorithm that are passed through the recipe parameters.

Please, try to think about what should be in the documentation for the
user, not the developer.

Sure, I will work on more user-centric documentation and fix aforementioned issues. 


>         """
>         ipsec_mode = self.params.ipsec_mode
>         spi_values = self.spi_values
>@@ -144,7 +150,7 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
>     def apply_sub_configuration(self, config):
>         """
>         Subconfiguration containing IPsec tunnel is applied through
>-        XfrmTools class.
>+        :any:`XfrmTools` class' methods using the previously generated configuration.
>         """
>         super().apply_sub_configuration(config)
>         ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns
>@@ -162,7 +168,7 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
>     def generate_ping_configurations(self, config):
>         """
>         The ping endpoints for this recipe are the configured endpoints of
>-        the IPsec tunnel on both hosts.
>+        the IPsec on both hosts.

Not sure why you omitted the tunnel word? I think it's ok to keep it.

I deleted the "tunnel" due to the IPsec's mode "tunnel" which might be confusing. We test both modes -
tunnel and transport and it might mislead the user to think we actually only test tunnel mode.
 

>         """
>         ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns
>         ip1, ip2 = config.ips
>@@ -180,8 +186,9 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
>
>     def generate_flow_combinations(self, config):
>         """
>-        Flow combinations are generated based on the tunnel endpoints
>-        and test parameters.
>+        Flow combinations are generated based on the IPsec endpoints
>+        and test parameters specified. All combinations are looped over
>+        and each flow is yielded.
>         """
>         nic1, nic2 = config.endpoint1, config.endpoint2
>         ns1, ns2 = config.endpoint1.netns, config.endpoint2.netns
>@@ -211,7 +218,9 @@ class IpsecEspAeadRecipe(CommonHWSubConfigMixin, BaseEnrtRecipe,
>
>     def ping_test(self, ping_configs):
>         """
>-        Ping test is utilizing PacketAssert class to search
>+        Ping test method overrides method of class :any:`PingTestAndEvaluate`.
>+        Method executes ping test according to the ping configuration through
>+        the IPsec with the use of :any:`PacketAssert` class to search
>         for the appropriate ESP IP packet. Result of ping
>         test is handed to the super class' method.
>
>--
>2.25.4
>_______________________________________________
>LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org
>To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org
>Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahosted.org