fix: inconsistent return values in github migrator (#7035)

* fix: inconsistent return values in github migrator

* feat: check setting value before updating and report correct status

* fix linter issues

awx/main/tests/unit/utils/test_github_migrator.py

@@ -0,0 +1,124 @@
+"""
+Unit tests for GitHub authenticator migrator functionality.
+"""
+
+from unittest.mock import Mock, patch
+from awx.sso.utils.github_migrator import GitHubMigrator
+
+
+class TestGitHubMigrator:
+    """Tests for GitHubMigrator class."""
+
+    def setup_method(self):
+        """Set up test fixtures."""
+        self.gateway_client = Mock()
+        self.command = Mock()
+        self.migrator = GitHubMigrator(self.gateway_client, self.command)
+
+    def test_create_gateway_authenticator_returns_boolean_causes_crash(self):
+        """
+        Test that verifies create_gateway_authenticator returns proper dictionary
+        structure instead of boolean when credentials are missing.
+
+        This test verifies the fix for the bug.
+        """
+        # Mock the get_controller_config to return a GitHub config with missing credentials
+        github_config_missing_creds = {
+            'category': 'github',
+            'settings': {'SOCIAL_AUTH_GITHUB_KEY': '', 'SOCIAL_AUTH_GITHUB_SECRET': 'test-secret'},  # Missing key
+            'org_mappers': [],
+            'team_mappers': [],
+            'login_redirect_override': None,
+        }
+
+        with patch.object(self.migrator, 'get_controller_config', return_value=[github_config_missing_creds]):
+            with patch.object(self.migrator, '_write_output'):  # Mock output to avoid noise
+                # This should NOT crash now that the bug is fixed
+                result = self.migrator.migrate()
+
+                # Verify the migration ran successfully without crashing
+                assert 'created' in result
+                assert 'failed' in result
+                # Should have failed=1 since the config has success=False (missing credentials)
+                assert result['failed'] == 1
+
+    def test_create_gateway_authenticator_returns_boolean_with_unknown_category(self):
+        """
+        Test that verifies create_gateway_authenticator returns proper dictionary
+        structure instead of boolean when category is unknown.
+
+        This test verifies the fix for the bug.
+        """
+        # Mock the get_controller_config to return a GitHub config with unknown category
+        github_config_unknown_category = {
+            'category': 'unknown-category',
+            'settings': {'SOCIAL_AUTH_UNKNOWN_KEY': 'test-key', 'SOCIAL_AUTH_UNKNOWN_SECRET': 'test-secret'},
+            'org_mappers': [],
+            'team_mappers': [],
+            'login_redirect_override': None,
+        }
+
+        with patch.object(self.migrator, 'get_controller_config', return_value=[github_config_unknown_category]):
+            with patch.object(self.migrator, '_write_output'):  # Mock output to avoid noise
+                # This should NOT crash now that the bug is fixed
+                result = self.migrator.migrate()
+
+                # Verify the migration ran successfully without crashing
+                assert 'created' in result
+                assert 'failed' in result
+                # Should have failed=1 since the config has success=False (unknown category)
+                assert result['failed'] == 1
+
+    def test_create_gateway_authenticator_direct_boolean_return_missing_creds(self):
+        """
+        Test that directly calls create_gateway_authenticator and verifies it returns
+        proper dictionary structure instead of boolean for missing credentials.
+        """
+        # Config with missing key (empty string)
+        config_missing_key = {
+            'category': 'github',
+            'settings': {'SOCIAL_AUTH_GITHUB_KEY': '', 'SOCIAL_AUTH_GITHUB_SECRET': 'test-secret'},  # Missing key
+            'org_mappers': [],
+            'team_mappers': [],
+            'login_redirect_override': None,
+        }
+
+        with patch.object(self.migrator, '_write_output'):  # Mock output to avoid noise
+            result = self.migrator.create_gateway_authenticator(config_missing_key)
+
+            # Now the method should return a proper dictionary structure
+            assert isinstance(result, dict), f"Expected dict, got {type(result)} with value: {result}"
+            assert 'success' in result, f"Expected 'success' key in result: {result}"
+            assert 'action' in result, f"Expected 'action' key in result: {result}"
+            assert 'error' in result, f"Expected 'error' key in result: {result}"
+            # Verify the expected values
+            assert result['success'] is False
+            assert result['action'] == 'skipped'
+            assert 'Missing OAuth2 credentials' in result['error']
+
+    def test_create_gateway_authenticator_direct_boolean_return_unknown_category(self):
+        """
+        Test that directly calls create_gateway_authenticator and verifies it returns
+        proper dictionary structure instead of boolean for unknown category.
+        """
+        # Config with unknown category
+        config_unknown_category = {
+            'category': 'unknown-category',
+            'settings': {'SOCIAL_AUTH_UNKNOWN_KEY': 'test-key', 'SOCIAL_AUTH_UNKNOWN_SECRET': 'test-secret'},
+            'org_mappers': [],
+            'team_mappers': [],
+            'login_redirect_override': None,
+        }
+
+        with patch.object(self.migrator, '_write_output'):  # Mock output to avoid noise
+            result = self.migrator.create_gateway_authenticator(config_unknown_category)
+
+            # Now the method should return a proper dictionary structure
+            assert isinstance(result, dict), f"Expected dict, got {type(result)} with value: {result}"
+            assert 'success' in result, f"Expected 'success' key in result: {result}"
+            assert 'action' in result, f"Expected 'action' key in result: {result}"
+            assert 'error' in result, f"Expected 'error' key in result: {result}"
+            # Verify the expected values
+            assert result['success'] is False
+            assert result['action'] == 'skipped'
+            assert 'Unknown category unknown-category' in result['error']

awx/sso/utils/github_migrator.py

@@ -124,7 +124,7 @@ class GitHubMigrator(BaseAuthenticatorMigrator):
 
         if not key_value or not secret_value:
             self._write_output(f'Skipping {category}: missing OAuth2 credentials', 'warning')
-            return False
+            return {'success': False, 'action': 'skipped', 'error': 'Missing OAuth2 credentials'}
 
         # Generate authenticator name and slug
         authenticator_name = category
@@ -143,7 +143,7 @@ class GitHubMigrator(BaseAuthenticatorMigrator):
         authenticator_type = type_mapping.get(category)
         if not authenticator_type:
             self._write_output(f'Unknown category {category}, skipping', 'warning')
-            return False
+            return {'success': False, 'action': 'skipped', 'error': f'Unknown category {category}'}
 
         self._write_output(f'\n--- Processing {category} authenticator ---')
         self._write_output(f'Name: {authenticator_name}')

awx/sso/utils/settings_migrator.py

@@ -96,11 +96,21 @@ class SettingsMigrator(BaseAuthenticatorMigrator):
         setting_value = config['setting_value']
 
         self._write_output(f'\n--- Migrating setting: {setting_name} ---')
-        self._write_output(f'New value: {setting_value}')
 
         try:
             gateway_setting_name = self._convert_setting_name(setting_name)
 
+            # Get current gateway setting value to check if update is needed
+            current_gateway_value = self.gateway_client.get_gateway_setting(gateway_setting_name)
+
+            # Compare current gateway value with controller value
+            if current_gateway_value == setting_value:
+                self._write_output(f'↷ Setting unchanged: {setting_name} (value already matches)', 'warning')
+                return {'success': True, 'action': 'skipped', 'error': None}
+
+            self._write_output(f'Current value: {current_gateway_value}')
+            self._write_output(f'New value: {setting_value}')
+
             # Use the new update_gateway_setting method
             self.gateway_client.update_gateway_setting(gateway_setting_name, setting_value)