sts-sponsors team mailing list archive

[Jack Lloyd-Walters <mp+434404@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

Inline nit, looks good elsewise

Diff comments:

> diff --git a/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py b/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py
> index 4964f08..cb721cb 100755
> --- a/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py
> +++ b/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py
> @@ -226,7 +226,7 @@ class IPMIBase(BMCConfig):
>      def _bmc_get_config(self, section=None):
>          """Fetch and cache all BMC settings."""
>          print("INFO: Reading current IPMI BMC values...")
> -        cmd = ["bmc-config", "--checkout"]
> +        cmd = ["bmc-config", "--checkout --verbose"]

strictly speaking this should be `cmd = ["bmc-config", "--checkout", "--verbose"]`

>          if section:
>              cmd += ["-S", section]
>          try:
> diff --git a/src/metadataserver/builtin_scripts/commissioning_scripts/tests/test_bmc_config.py b/src/metadataserver/builtin_scripts/commissioning_scripts/tests/test_bmc_config.py
> index 1fced30..2b29a9d 100644
> --- a/src/metadataserver/builtin_scripts/commissioning_scripts/tests/test_bmc_config.py
> +++ b/src/metadataserver/builtin_scripts/commissioning_scripts/tests/test_bmc_config.py
> @@ -490,72 +490,83 @@ EndSection
>          self.assertRaises(SystemExit, self.ipmi.add_bmc_user)
>  
>      def test_set_ipmi_lan_channel_setting_verifies(self):
> -        self.ipmi._bmc_config = {
> -            "Lan_Channel": {
> -                "Volatile_Access_Mode": "Always_Available",
> -                "Non_Volatile_Access_Mode": "Always_Available",
> +
> +        for channel in [
> +            "Lan_Channel",
> +            "Lan_Channel_Channel_1",
> +            "Lan_Channel_Channel_2",
> +            "Lan_Channel_Channel_3",
> +        ]:
> +            self.ipmi._bmc_config = {
> +                channel: {
> +                    "Volatile_Access_Mode": "Always_Available",
> +                    "Non_Volatile_Access_Mode": "Always_Available",
> +                },
>              }
> -        }
> -        mock_bmc_set = self.patch(self.ipmi, "_bmc_set")
> -        mock_bmc_set_keys = self.patch(self.ipmi, "_bmc_set_keys")
> -        self.ipmi._config_ipmi_lan_channel_settings()
> -        self.assertThat(mock_bmc_set, MockNotCalled())
> -        self.assertThat(
> -            mock_bmc_set_keys,
> -            MockCalledOnceWith(
> -                "Lan_Channel",
> -                [
> -                    f"{auth_type}_{volatility}"
> -                    for auth_type in [
> -                        "Enable_User_Level_Auth",
> -                        "Enable_Per_Message_Auth",
> -                        "Enable_Pef_Alerting",
> -                    ]
> -                    for volatility in ["Volatile", "Non_Volatile"]
> -                ],
> -                "Yes",
> -            ),
> -        )
> +            mock_bmc_set = self.patch(self.ipmi, "_bmc_set")
> +            mock_bmc_set_keys = self.patch(self.ipmi, "_bmc_set_keys")
> +            self.ipmi._config_ipmi_lan_channel_settings()
> +            self.assertThat(mock_bmc_set, MockNotCalled())

We're trying to move away from testtools, could you replace this with tests using just the unittests library of assertions?

> +            self.assertThat(

same with this

> +                mock_bmc_set_keys,
> +                MockCalledOnceWith(
> +                    channel,
> +                    [
> +                        f"{auth_type}_{volatility}"
> +                        for auth_type in [
> +                            "Enable_User_Level_Auth",
> +                            "Enable_Per_Message_Auth",
> +                            "Enable_Pef_Alerting",
> +                        ]
> +                        for volatility in ["Volatile", "Non_Volatile"]
> +                    ],
> +                    "Yes",
> +                ),
> +            )
>  
>      def test_set_ipmi_lan_channel_setting_enables(self):
> -        self.ipmi._bmc_config = {
> -            "Lan_Channel": {
> -                "Volatile_Access_Mode": "Disabled",
> -                "Non_Volatile_Access_Mode": "Pre_Boot_only",
> +        for channel in [
> +            "Lan_Channel",
> +            "Lan_Channel_Channel_1",
> +            "Lan_Channel_Channel_2",
> +            "Lan_Channel_Channel_3",
> +        ]:
> +            self.ipmi._bmc_config = {
> +                channel: {
> +                    "Volatile_Access_Mode": "Disabled",
> +                    "Non_Volatile_Access_Mode": "Pre_Boot_only",
> +                },
>              }
> -        }
> -        mock_bmc_set = self.patch(self.ipmi, "_bmc_set")
> -        mock_bmc_set_keys = self.patch(self.ipmi, "_bmc_set_keys")
> -        self.ipmi._config_ipmi_lan_channel_settings()
> -        self.assertThat(
> -            mock_bmc_set,
> -            MockCallsMatch(
> -                call(
> -                    "Lan_Channel", "Volatile_Access_Mode", "Always_Available"
> +            mock_bmc_set = self.patch(self.ipmi, "_bmc_set")
> +            mock_bmc_set_keys = self.patch(self.ipmi, "_bmc_set_keys")
> +            self.ipmi._config_ipmi_lan_channel_settings()
> +            self.assertThat(

and here

> +                mock_bmc_set,
> +                MockCallsMatch(
> +                    call(channel, "Volatile_Access_Mode", "Always_Available"),
> +                    call(
> +                        channel,
> +                        "Non_Volatile_Access_Mode",
> +                        "Always_Available",
> +                    ),
>                  ),
> -                call(
> -                    "Lan_Channel",
> -                    "Non_Volatile_Access_Mode",
> -                    "Always_Available",
> +            )
> +            self.assertThat(

assertion here

> +                mock_bmc_set_keys,
> +                MockCalledOnceWith(
> +                    channel,
> +                    [
> +                        f"{auth_type}_{volatility}"
> +                        for auth_type in [
> +                            "Enable_User_Level_Auth",
> +                            "Enable_Per_Message_Auth",
> +                            "Enable_Pef_Alerting",
> +                        ]
> +                        for volatility in ["Volatile", "Non_Volatile"]
> +                    ],
> +                    "Yes",
>                  ),
> -            ),
> -        )
> -        self.assertThat(
> -            mock_bmc_set_keys,
> -            MockCalledOnceWith(
> -                "Lan_Channel",
> -                [
> -                    f"{auth_type}_{volatility}"
> -                    for auth_type in [
> -                        "Enable_User_Level_Auth",
> -                        "Enable_Per_Message_Auth",
> -                        "Enable_Pef_Alerting",
> -                    ]
> -                    for volatility in ["Volatile", "Non_Volatile"]
> -                ],
> -                "Yes",
> -            ),
> -        )
> +            )
>  
>      def test_config_lan_conf_auth(self):
>          self.ipmi._bmc_config = {"Lan_Channel_Auth": {}}


-- 
https://code.launchpad.net/~seyeongkim/maas/+git/maas/+merge/434404
Your team MAAS Committers is subscribed to branch maas:master.