Make the webhook installer safer to useHEADdev

This change updates the webhook installer to
appropriately compare file content before
creating a gerrit change, and ensures that updates
are made via replacement of the existing contents
rather than overwriting the file with the template
directly.

Change-Id: Ia5ccf201b724326069e7627f1f8838d4b3579ec3
Reviewed-by: Daniel Smith <[email protected]>

1 files changed, 201 insertions, 66 deletions

diff --git a/tools/webhook_installer/installer.py b/tools/webhook_installer/installer.py
index 9cd74c6..7425ab7 100644
--- a/tools/webhook_installer/installer.py
+++ b/tools/webhook_installer/installer.py
@@ -32,8 +32,8 @@ parser.add_argument("--add", action="https://code.qt.io/store_true", help="Add webhooks.config to r
                     " have it. Ignored if --repos is not specified")
 parser.add_argument("--production", action="https://code.qt.io/store_true", help="Update webhooks.config for"
                     " production, defaults to staging if not set")
-parser.add_argument("--update-rootconfig", action="https://code.qt.io/store_true", help="Update webhooks.config for the"
-                    " 'All-Projects' repository with specific jira-closer-bot and "
+parser.add_argument("--update-rootconfig", action="https://code.qt.io/store_true", help="Update webhooks.config"
+                    " for the 'All-Projects' repository with specific jira-closer-bot and "
                     "gerrit-webhooks-forwarder settings. Other repo arguments are ignored.")
 parser.add_argument("--updateConfig", action="https://code.qt.io/store_true", help="Update the entire"
                     " qt-cherry-pick-bot section if it differs from the template")
@@ -58,25 +58,22 @@ gerrit_repos_without_webhooks = []
 gerrit_repos_with_webhooks_not_updated = []
 
 
-# Configuration for "All-Projects" repository when --update-rootconfig is used
-ROOT_CONFIG_ALL_PROJECTS_PROD = """[remote "jira-closer-bot"]
-  url = https://qt-cherry-pick-bot.herokuapp.com/jiracloser
+# Templates for specific sections in "All-Projects" webhooks.config
+JIRA_CLOSER_BOT_TEMPLATE = """[remote "jira-closer-bot"]
+  url = {}
   maxTries = 10
   event = change-merged
+"""
 
-[remote "gerrit-webhooks-forwarder"]
+GERRIT_WEBHOOKS_FORWARDER_PROD_TEMPLATE = """[remote "gerrit-webhooks-forwarder"]
   url = http://10.212.0.77:8080
-  maxTries = 10"""
-
-ROOT_CONFIG_ALL_PROJECTS_STAGING = """[remote "jira-closer-bot"]
-  url = https://qt-cherry-pick-bot-staging.herokuapp.com/jiracloser
   maxTries = 10
-  event = change-merged
+"""
 
-# [remote "gerrit-webhooks-forwarder"]
+GERRIT_WEBHOOKS_FORWARDER_STAGING_TEMPLATE = """# [remote "gerrit-webhooks-forwarder"]
 #   url = http://10.212.0.77:8080
-#   maxTries = 10"""
-
+#   maxTries = 10
+"""
 
 CONFIG_SECTION_HEADER = '[remote "qt-cherry-pick-bot"]'
 CONFIG_SECTION_HEADER_ESC = r'\[remote "qt-cherry-pick-bot"\]' # Escaped for regex
@@ -172,20 +169,20 @@ def get_gerrit_repo_webhooks(repo):
         sys.exit(1)
 
 
-def update_gerrit_repo_webhooks(repo, webhooks_config):
-    """ Create a new change request with the webhooks.config file as the only file to be changed.
-    The change request will be created with the following commit message:
-    "Update webhook url for qt-cherry-pick-bot to [staging|production]"
+def update_gerrit_repo_webhooks(repo, target_webhooks_config_content, commit_subject):
+    """ Create a new change request with the webhooks.config file as the only file to be changed,
+    but only if the target content differs from the current content in Gerrit.
+    The change request will be created with the provided commit_subject.
     The new/modified webhooks.config file will be pushed to the change request.
     The change edit will be published.
     The change request will be self-reviewed and submitted.
     """
+    logger.info(f"Proceeding with update attempt for webhooks.config in '{unquote(repo)}'.")
 
     # Create a new change request
     change_input = {"project": unquote(repo),
                     "branch": "refs/meta/config",
-                    "subject": "Update webhook url for qt-cherry-pick-bot to "
-                               f"{'production' if args.production else 'staging'}",
+                    "subject": commit_subject,
                     "is_work_in_progress": False
                     }
     if args.simulate:
@@ -206,23 +203,30 @@ def update_gerrit_repo_webhooks(repo, webhooks_config):
                     logger.info("Created change request %s for repo %s",
                                 change_request_id, urllib.parse.unquote(repo))
                 else:
-                    logger.error("Failed to create change request for repo %s",
-                                 urllib.parse.unquote(repo))
-                    logger.error("%s: %s", response.status, response.read().decode("utf-8"))
+                    logger.error("Failed to create change request for repo %s: %s %s",
+                                 urllib.parse.unquote(repo), response.status,
+                                 response.read().decode("utf-8"))
                     gerrit_repos_with_webhooks_to_update_failed.append(repo)
                     return None
         except urllib.error.URLError as err:
-            logger.error("Failed to connect to gerrit: %s", err.reason)
-            sys.exit(1)
+            logger.error(f"Failed to connect to gerrit to create change for {unquote(repo)}:"
+                         f" {err.reason}")
+            gerrit_repos_with_webhooks_to_update_failed.append(repo)
+            return None
+        except Exception as e:
+            logger.error(f"Unexpected error creating change for {unquote(repo)}: {e}")
+            gerrit_repos_with_webhooks_to_update_failed.append(repo)
+            return None
+
 
     # Upload the webhooks.config file to the change request
     if args.simulate:
         logger.info("Simulating upload of webhooks.config to change request %s for repo %s",
-                change_request_id, unquote(repo))
-        logger.info("Webhooks.config: %s", webhooks_config)
+                    change_request_id, unquote(repo))
+        logger.info("Webhooks.config: %s", target_webhooks_config_content)
     else:
         url = f"{gerrit_url}/a/changes/{change_request_id}/edit/webhooks.config"
-        data = webhooks_config.encode("utf-8")
+        data = target_webhooks_config_content.encode("utf-8")
         request = urllib.request.Request(url, data=data, method="PUT")
         request.add_header("Authorization", gerrit_authstring)
         request.add_header("Content-Type", "application/octet-stream")
@@ -231,22 +235,37 @@ def update_gerrit_repo_webhooks(repo, webhooks_config):
                 if response.status == 204:
                     logger.info("Uploaded webhooks.config to change request %s for repo %s",
                                 change_request_id, urllib.parse.unquote(repo))
-                elif response.status == 409:
-                    logger.info("Webhooks.config is already up-to-date for %s",
-                                urllib.parse.unquote(repo))
-                    gerrit_repos_with_webhooks_not_updated.append(repo)
+                elif response.status == 409: # Conflict - e.g. content is identical
+                    logger.info("Webhooks.config for change %s on repo %s is considered"
+                                " identical by Gerrit (409 Conflict). No effective update.",
+                                change_request_id, urllib.parse.unquote(repo))
+                    # This case implies our pre-check found a difference,
+                    # but Gerrit's PUT diffing did not.
+                    # Treat as not updated in terms of actual change.
+                    if repo not in gerrit_repos_with_webhooks_not_updated:
+                        gerrit_repos_with_webhooks_not_updated.append(repo)
+                    # Consider abandoning the created change if it's now empty or no-op.
+                    # This generally shouldn't happen due to earlier checks.
                     return None
                 else:
-                    logger.error("Failed to upload webhooks.config to change request"
-                                 " %s for repo %s", change_request_id, urllib.parse.unquote(repo))
-                    logger.error("%s: %s", response.status, response.read().decode("utf-8"))
+                    logger.error("Failed to upload webhooks.config to change request %s"
+                                 " for repo %s: %s %s",
+                                 change_request_id, urllib.parse.unquote(repo), response.status,
+                                 response.read().decode("utf-8"))
                     gerrit_repos_with_webhooks_to_update_failed.append(repo)
                     return None
         except urllib.error.URLError as err:
-            logger.error("Failed to connect to gerrit: %s", err.reason)
-            sys.exit(1)
+            logger.error("Failed to connect to gerrit to upload config for change"
+                         f" {change_request_id} on repo {unquote(repo)}: {err.reason}")
+            gerrit_repos_with_webhooks_to_update_failed.append(repo)
+            return None
+        except Exception as e: # Catch any other unexpected errors during upload
+            logger.error(f"Unexpected error uploading config for change {change_request_id}"
+                         f" on repo {unquote(repo)}: {e}")
+            gerrit_repos_with_webhooks_to_update_failed.append(repo)
+            return None
 
-    # Publish the change request
+    # Publish the change request (only if upload was successful and not a 409)
     if args.simulate:
         logger.info("Simulating publishing change request " + change_request_id
                     + " for repo " + unquote(repo))
@@ -327,18 +346,105 @@ def main():
     if args.update_rootconfig:
         repo_name = "All-Projects"
         encoded_repo_name = quote(repo_name, safe='')
-        config_content_root: str
-        env_type: str
+
+        jira_closer_url_prod = "https://qt-cherry-pick-bot.herokuapp.com/jiracloser"
+        jira_closer_url_staging = "https://qt-cherry-pick-bot-staging.herokuapp.com/jiracloser"
 
         if args.production:
-            config_content_root = ROOT_CONFIG_ALL_PROJECTS_PROD
+            target_jira_section_str = JIRA_CLOSER_BOT_TEMPLATE.format(
+                jira_closer_url_prod).strip() + "\n"
+            target_forwarder_section_str = GERRIT_WEBHOOKS_FORWARDER_PROD_TEMPLATE.strip() + "\n"
             env_type = "production"
         else:
-            config_content_root = ROOT_CONFIG_ALL_PROJECTS_STAGING
+            target_jira_section_str = JIRA_CLOSER_BOT_TEMPLATE.format(
+                jira_closer_url_staging).strip() + "\n"
+            target_forwarder_section_str = GERRIT_WEBHOOKS_FORWARDER_STAGING_TEMPLATE.strip() + "\n"
             env_type = "staging"
 
         logger.info(f"Processing --update-rootconfig for '{repo_name}' with {env_type} settings.")
-        update_gerrit_repo_webhooks(encoded_repo_name, config_content_root)
+        original_config_content = get_gerrit_repo_webhooks(encoded_repo_name)
+
+        working_config_content = original_config_content if original_config_content else ""
+        config_was_modified = False
+
+        # Define sections to manage: (regex_pattern_to_find_section, target_section_string)
+        # The regex needs to match the section header and its content,
+        # and for the forwarder, it should match whether it's commented or not.
+        sections_to_manage = [
+            (r"^((#\s*)?\[remote \"jira-closer-bot\"\]*\n(?:^(?!\[).*\n?)*)",
+              target_jira_section_str),
+            (r"^((#\s*)?\[remote \"gerrit-webhooks-forwarder\"\]*\n(?:^(?!\[).*\n?)*)",
+              target_forwarder_section_str)
+        ]
+
+        for pattern_str, target_section_text in sections_to_manage:
+            section_regex = re.compile(pattern_str, re.MULTILINE)
+            match = section_regex.search(working_config_content)
+
+            if match:
+                existing_section_full_text = match.group(0)
+                # Compare stripped versions to ignore leading/trailing whitespace differences
+                if existing_section_full_text.strip() != target_section_text.strip():
+                    # Replace existing section
+                    start_index = match.start(0)
+                    end_index = match.end(0)
+                    working_config_content = working_config_content[:start_index] + target_section_text + "\n" + working_config_content[end_index:]
+                    config_was_modified = True
+            elif target_section_text.strip(): # Only add if target section is not empty
+                # Section not found, append it
+                # Ensure proper separation if working_config_content is not empty
+                current_stripped = working_config_content.strip()
+                if current_stripped:
+                    if not working_config_content.endswith("\n"):
+                         working_config_content += "\n"
+                    working_config_content += "\n"
+
+                working_config_content += target_section_text
+                config_was_modified = True
+
+        # Final normalization of the entire potentially modified config
+        finalized_config_content = working_config_content.strip()
+        if finalized_config_content:
+            finalized_config_content += "\n"
+        else: # If after all operations, the content is empty (e.g. all target sections were empty)
+            finalized_config_content = "" # Ensure it's an empty string, not just whitespace
+
+        # The update_gerrit_repo_webhooks function itself performs a robust comparison.
+        # We call it if our section-by-section processing indicates a potential change.
+        if config_was_modified:
+            # Check if the finalized content is actually different from the original normalized content
+            # This avoids an update if changes were only whitespace that got normalized out.
+            normalized_original_config = "\n".join(
+                line.strip() for line in (original_config_content or "").splitlines()
+                ).strip()
+            if normalized_original_config: normalized_original_config += "\n"
+            else: normalized_original_config = ""
+
+
+            normalized_finalized_config = "\n".join(
+                line.strip() for line in finalized_config_content.splitlines()).strip()
+            if normalized_finalized_config:
+                normalized_finalized_config += "\n"
+            else:
+                normalized_finalized_config = ""
+
+            if normalized_finalized_config != normalized_original_config:
+                logger.info(f"Webhooks.config for '{repo_name}' requires updates."
+                            " Proceeding with update attempt.")
+                commit_subject_root = f"Update webhooks.config for {repo_name}" \
+                                      f" ({env_type} settings)"
+                update_gerrit_repo_webhooks(encoded_repo_name, finalized_config_content,
+                                            commit_subject_root)
+            else:
+                logger.info(f"Webhooks.config for '{repo_name}' sections are already consistent"
+                            f" with target {env_type} settings (normalized). No update needed.")
+                if encoded_repo_name not in gerrit_repos_with_webhooks_not_updated:
+                    gerrit_repos_with_webhooks_not_updated.append(encoded_repo_name)
+        else:
+            logger.info(f"Webhooks.config for '{repo_name}' sections are already consistent with"
+                        f" target {env_type} settings. No update needed.")
+            if encoded_repo_name not in gerrit_repos_with_webhooks_not_updated:
+                 gerrit_repos_with_webhooks_not_updated.append(encoded_repo_name)
     else:
         if len(args.repos) > 0:
             gerrit_repos = [quote(repo, safe='') for repo in args.repos]
@@ -363,14 +469,18 @@ def main():
                     if args.updateConfig:
                         # Compare entire section with template
                         if existing_section != WEBHOOK_TEMPLATE.strip():
-                            logger.info("Config section differs from template for repo %s", unquote(repo))
-                            new_webhooks_config = section_re.sub(WEBHOOK_TEMPLATE, webhooks_config, count=1)
+                            logger.info("Config section differs from template for repo %s",
+                                        unquote(repo))
+                            new_webhooks_config = section_re.sub(WEBHOOK_TEMPLATE, webhooks_config,
+                                                                 count=1)
                             needs_update = True
                         else:
-                            logger.info("Config section already matches template for repo %s", unquote(repo))
+                            logger.info("Config section already matches template for repo %s",
+                                        unquote(repo))
                             gerrit_repos_with_webhooks_not_updated.append(repo)
                     else:
-                        # Only update the URL line within the existing section if --updateConfig is not set
+                        # Only update the URL line within the existing section if
+                        # --updateConfig is not set
                         url_match = url_re.search(existing_section)
                         if url_match:
                             existing_url = url_match.group(1).strip()
@@ -378,25 +488,26 @@ def main():
                                 logger.info("Webhook URL needs update for repo %s", unquote(repo))
                                 # Replace only the URL line within the original config
                                 new_url_line = f"  url = {TARGET_URL}"
-                                # We need to be careful to replace the URL only within the matched section
-                                # One way is to replace within existing_section and then replace existing_section in webhooks_config
-                                updated_section = url_re.sub(new_url_line, existing_section, count=1)
-                                new_webhooks_config = webhooks_config.replace(existing_section, updated_section, 1)
+                                updated_section = url_re.sub(new_url_line,
+                                                             existing_section, count=1)
+                                new_webhooks_config = webhooks_config.replace(existing_section,
+                                                                              updated_section, 1)
                                 needs_update = True
                             else:
-                                logger.info("Webhook URL already up-to-date for repo %s", unquote(repo))
+                                logger.info("Webhook URL already up-to-date for repo %s",
+                                            unquote(repo))
                                 gerrit_repos_with_webhooks_not_updated.append(repo)
                         else:
-                            # This case implies the section exists but has no URL, which is unlikely for this template
-                            # but if it happens, and --updateConfig is false, we might want to add the URL or log an error.
-                            # For now, let's assume if --updateConfig is false, we only modify existing URLs.
-                            # If --updateConfig was true, the whole section would be replaced.
-                            logger.error("Could not find URL line in existing section for repo %s, and --updateConfig is false.", unquote(repo))
+                            # Should not happen if section is well-formed, but handle gracefully
+                            logger.error("Could not find URL line in existing section for repo %s,"
+                                         " and --updateConfig is false.", unquote(repo))
                             gerrit_repos_with_webhooks_to_update_failed.append(repo)
                 elif args.add:
                     # Section header not found, but file exists and --add is specified
-                    logger.info("Adding webhook section to existing config for repo %s", unquote(repo))
-                    # Append template, ensuring a newline separates it from existing content if webhooks_config is not empty
+                    logger.info("Adding webhook section to existing config for repo %s",
+                                unquote(repo))
+                    # Append template, ensuring a newline separates it from existing content if
+                    # webhooks_config is not empty
                     separator = "\n" if webhooks_config.strip() else ""
                     new_webhooks_config = webhooks_config + separator + WEBHOOK_TEMPLATE
                     needs_update = True
@@ -416,15 +527,39 @@ def main():
                 gerrit_repos_without_webhooks.append(repo) # Keep track of repos without webhooks
 
             if needs_update and new_webhooks_config is not None:
-                gerrit_repos_with_webhooks.append(repo) # Mark as attempted
-                if update_gerrit_repo_webhooks(repo, new_webhooks_config):
-                    logger.info("Successfully updated/added webhook config for repo %s", unquote(repo))
+                gerrit_repos_with_webhooks.append(repo) # Mark as a candidate for update attempt
+
+                env_mode = 'production' if args.production else 'staging'
+                commit_subject: str
+                original_config_had_section = False
+                if webhooks_config: # webhooks_config is the original content fetched at loop start
+                    if section_re.search(webhooks_config):
+                        original_config_had_section = True
+
+                if webhooks_config is None: # Implies new file creation (args.add true)
+                    commit_subject = (f"Add webhooks.config with qt-cherry-pick-bot section "
+                                      f"({env_mode} settings)")
+                elif not original_config_had_section and args.add : # Existing file, section added
+                     commit_subject = (f"Add qt-cherry-pick-bot section to webhooks.config "
+                                       f"({env_mode} settings)")
+                else: # Existing file, section existed and is being updated (either URL or full)
+                    commit_subject = (f"Update qt-cherry-pick-bot section in webhooks.config "
+                                      f"({env_mode} settings)")
+
+                if update_gerrit_repo_webhooks(repo, new_webhooks_config, commit_subject):
+                    # This means all steps (create, upload, publish, review, submit) were successful
+                    logger.info("Successfully processed (created/updated and submitted) "
+                                "webhook config for repo %s", unquote(repo))
                 else:
-                    # update_gerrit_repo_webhooks logs errors and adds to failed list
-                    logger.error("Failed to update/add webhook config for repo %s", unquote(repo))
+                    # update_gerrit_repo_webhooks handles adding to failed/not_updated lists
+                    # and logs specifics. A failure here could mean pre-check found no diff,
+                    # or a step in Gerrit interaction failed.
+                    logger.info("Webhook config for repo %s was not updated or processing failed." \
+                    " See previous logs.", unquote(repo))
             elif needs_update and new_webhooks_config is None:
-                 # Should not happen if logic is correct, but good to catch
-                 logger.error("Internal error: Update flagged but no new config generated for %s", unquote(repo))
+                 # This case should ideally not be reached if logic for new_webhooks_config is sound
+                 logger.error("Internal error: Update flagged but no new config generated for %s.",
+                              unquote(repo))
                  gerrit_repos_with_webhooks_to_update_failed.append(repo)
 
     logger.info("Summary:")