Skip to content

Commit 72c56c5

Browse files
committed
Changed invalid dnf options to fail appstreams promise
Invalid DNF options are configuration errors and should fail the promise as NOT_KEPT rather than logging warnings and continuing. Changes: - Renamed _try_apply_dnf_options -> _apply_dnf_options - Removed try/except wrapper inside _apply_dnf_options - Added try/except in callers to catch ConfigError and return NOT_KEPT - Added test_invalid_dnf_option_not_kept to verify behavior This aligns with strict configuration validation - if a user specifies an invalid option, it's a policy error that should be fixed.
1 parent a59fa1d commit 72c56c5

2 files changed

Lines changed: 43 additions & 14 deletions

File tree

promise-types/appstreams/appstreams.py

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -375,8 +375,8 @@ def _log_failed_packages(self, failed_packages):
375375
for pkg, error in failed_packages:
376376
self.log_error(f" Package {pkg} failed: {error}")
377377

378-
def _try_apply_dnf_options(self, base, options):
379-
"""Apply DNF configuration options at best effort (logs errors but continues)"""
378+
def _apply_dnf_options(self, base, options):
379+
"""Apply DNF configuration options, raising ConfigError on invalid options"""
380380
if not options:
381381
return
382382

@@ -386,24 +386,21 @@ def _try_apply_dnf_options(self, base, options):
386386
key = key.strip()
387387
value = value.strip()
388388

389-
try:
390-
# Use DNF's set_or_append_opt_value to handle options generically
391-
base.conf.set_or_append_opt_value(key, value)
392-
self.log_verbose(f"Set DNF option: {key}={value}")
393-
except dnf.exceptions.ConfigError as e:
394-
self.log_warning(f"Failed to set DNF option '{key}={value}': {e}")
395-
except Exception as e:
396-
self.log_warning(
397-
f"Unexpected error setting DNF option '{key}={value}': {e}"
398-
)
389+
# Raises dnf.exceptions.ConfigError if option is invalid
390+
base.conf.set_or_append_opt_value(key, value)
391+
self.log_verbose(f"Set DNF option: {key}={value}")
399392

400393
def _switch_module(self, mpc, base, module_name, stream, profile, options=None):
401394
"""Switch a module to a different stream using ModuleBase.switch_to()"""
402395
if options is None:
403396
options = []
404397

405398
# Apply DNF configuration options
406-
self._try_apply_dnf_options(base, options)
399+
try:
400+
self._apply_dnf_options(base, options)
401+
except dnf.exceptions.ConfigError as e:
402+
self.log_error(f"Invalid DNF option: {e}")
403+
return Result.NOT_KEPT
407404

408405
if not stream:
409406
self.log_error("Stream must be specified for module switch")
@@ -483,7 +480,11 @@ def _switch_module(self, mpc, base, module_name, stream, profile, options=None):
483480
def _install_module(self, mpc, base, module_name, stream, profile, options=None):
484481
"""Enable a module stream and install the given (or default) profile's packages."""
485482
# Apply DNF options if specified
486-
self._try_apply_dnf_options(base, options)
483+
try:
484+
self._apply_dnf_options(base, options)
485+
except dnf.exceptions.ConfigError as e:
486+
self.log_error(f"Invalid DNF option: {e}")
487+
return Result.NOT_KEPT
487488

488489
if not stream:
489490
try:

promise-types/appstreams/test_appstreams_logic.py

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,34 @@ def test_profile_default_not_found(module, mock_base, mock_mpc):
286286
assert result == Result.NOT_KEPT
287287

288288

289+
def test_invalid_dnf_option_not_kept(module, mock_base, mock_mpc):
290+
"""Test that invalid DNF options cause NOT_KEPT (ConfigError from DNF)"""
291+
from unittest.mock import PropertyMock
292+
293+
# Mock ConfigError to be raised when invalid option is set
294+
mock_base.conf.set_or_append_opt_value.side_effect = (
295+
mock_dnf.exceptions.ConfigError('Cannot set "invalid_option" to "value"')
296+
)
297+
298+
mock_mpc.getModuleState.return_value = mock_mpc.ModuleState_ENABLED
299+
mock_mpc.getEnabledStream.return_value = "12"
300+
mock_mpc.getInstalledProfiles.return_value = []
301+
302+
result = module.evaluate_promise(
303+
"nodejs",
304+
{
305+
"state": "installed",
306+
"stream": "12",
307+
"profile": "common",
308+
"options": ["invalid_option=value"],
309+
},
310+
{},
311+
)
312+
313+
# Should fail because invalid option raises ConfigError
314+
assert result == Result.NOT_KEPT
315+
316+
289317
def test_remove_unknown_module_runtime_error(module, mock_base, mock_mpc):
290318
"""Test removing a module when getEnabledStream raises RuntimeError"""
291319
mock_mpc.getModuleState.return_value = mock_mpc.ModuleState_ENABLED

0 commit comments

Comments
 (0)