feat: updater - set last validated commit after partial update

taf/tests/test_api/test_create_repository.py

@@ -41,58 +41,58 @@ def _check_repo_initialization_successful(auth_repo: AuthenticationRepository):
     assert commits[0].message.strip() == git_commit_message("create-repo")
 
 
-# def test_create_repository_when_no_delegations(
-#     auth_repo_path: Path, no_yubikeys_path: str, api_keystore: str
-# ):
-#     repo_path = str(auth_repo_path)
-#     create_repository(
-#         repo_path,
-#         roles_key_infos=no_yubikeys_path,
-#         keystore=api_keystore,
-#         commit=True,
-#     )
-
-#     auth_repo = AuthenticationRepository(path=repo_path)
-#     _check_repo_initialization_successful(auth_repo)
-#     assert auth_repo.is_test_repo is False
-#     validate_repository(repo_path)
-
-
-# def test_create_repository_when_no_delegations_with_test_flag(
-#     auth_repo_path: Path, no_yubikeys_path: str, api_keystore: str
-# ):
-#     repo_path = str(auth_repo_path)
-#     create_repository(
-#         repo_path,
-#         roles_key_infos=no_yubikeys_path,
-#         keystore=api_keystore,
-#         commit=True,
-#         test=True,
-#     )
-
-#     auth_repo = AuthenticationRepository(path=repo_path)
-#     _check_repo_initialization_successful(auth_repo)
-#     assert auth_repo.is_test_repo is True
-#     validate_repository(repo_path)
-
-
-# def test_create_repository_when_delegations(
-#     auth_repo_path: Path, with_delegations_no_yubikeys_path: str, api_keystore: str
-# ):
-#     repo_path = str(auth_repo_path)
-#     create_repository(
-#         str(auth_repo_path),
-#         roles_key_infos=with_delegations_no_yubikeys_path,
-#         keystore=api_keystore,
-#         commit=True,
-#     )
-
-#     auth_repo = AuthenticationRepository(path=auth_repo_path)
-#     _check_repo_initialization_successful(auth_repo)
-#     targets_roles = auth_repo.get_all_targets_roles()
-#     for role in ("targets", "delegated_role", "inner_role"):
-#         assert role in targets_roles
-#     validate_repository(repo_path)
+def test_create_repository_when_no_delegations(
+    auth_repo_path: Path, no_yubikeys_path: str, api_keystore: str
+):
+    repo_path = str(auth_repo_path)
+    create_repository(
+        repo_path,
+        roles_key_infos=no_yubikeys_path,
+        keystore=api_keystore,
+        commit=True,
+    )
+
+    auth_repo = AuthenticationRepository(path=repo_path)
+    _check_repo_initialization_successful(auth_repo)
+    assert auth_repo.is_test_repo is False
+    validate_repository(repo_path)
+
+
+def test_create_repository_when_no_delegations_with_test_flag(
+    auth_repo_path: Path, no_yubikeys_path: str, api_keystore: str
+):
+    repo_path = str(auth_repo_path)
+    create_repository(
+        repo_path,
+        roles_key_infos=no_yubikeys_path,
+        keystore=api_keystore,
+        commit=True,
+        test=True,
+    )
+
+    auth_repo = AuthenticationRepository(path=repo_path)
+    _check_repo_initialization_successful(auth_repo)
+    assert auth_repo.is_test_repo is True
+    validate_repository(repo_path)
+
+
+def test_create_repository_when_delegations(
+    auth_repo_path: Path, with_delegations_no_yubikeys_path: str, api_keystore: str
+):
+    repo_path = str(auth_repo_path)
+    create_repository(
+        str(auth_repo_path),
+        roles_key_infos=with_delegations_no_yubikeys_path,
+        keystore=api_keystore,
+        commit=True,
+    )
+
+    auth_repo = AuthenticationRepository(path=auth_repo_path)
+    _check_repo_initialization_successful(auth_repo)
+    targets_roles = auth_repo.get_all_targets_roles()
+    for role in ("targets", "delegated_role", "inner_role"):
+        assert role in targets_roles
+    validate_repository(repo_path)
 
 
 def test_create_repository_when_add_repositories_json(

taf/tests/test_updater/test_repo_update/test_updater.py

@@ -244,32 +244,47 @@ def test_no_update_necessary(
 
 
 @pytest.mark.parametrize(
-    "test_name, expected_error, auth_repo_name_exists, expect_partial_update",
+    "test_name, expected_error, auth_repo_name_exists, expect_partial_update, should_last_validated_exist",
     [
-        ("test-updater-invalid-target-sha", TARGET_MISSMATCH_PATTERN, True, True),
+        ("test-updater-invalid-target-sha", TARGET_MISSMATCH_PATTERN, True, True, True),
         (
             "test-updater-additional-target-commit",
             TARGET_COMMIT_AFTER_LAST_VALIDATED,
             True,
             True,
+            True,
+        ),
+        (
+            "test-updater-missing-target-commit",
+            TARGET_ADDITIONAL_COMMIT,
+            True,
+            True,
+            True,
         ),
-        ("test-updater-missing-target-commit", TARGET_ADDITIONAL_COMMIT, True, True),
-        ("test-updater-wrong-key", NO_WORKING_MIRRORS, True, True),
-        ("test-updater-invalid-version-number", REPLAYED_METADATA, True, True),
+        ("test-updater-wrong-key", NO_WORKING_MIRRORS, True, True, False),
+        ("test-updater-invalid-version-number", REPLAYED_METADATA, True, True, False),
         (
             "test-updater-delegated-roles-wrong-sha",
             TARGET_MISSMATCH_PATTERN,
             True,
             True,
+            True,
+        ),
+        # ("test-updater-updated-root-n-root-missing", NO_WORKING_MIRRORS, True, True, False),
+        (
+            "test-updater-updated-root-invalid-metadata",
+            NO_WORKING_MIRRORS,
+            True,
+            True,
+            False,
         ),
-        ("test-updater-updated-root-n-root-missing", NO_WORKING_MIRRORS, True, True),
-        ("test-updater-updated-root-invalid-metadata", NO_WORKING_MIRRORS, True, True),
-        ("test-updater-info-missing", NO_REPOSITORY_INFO_JSON, False, True),
+        ("test-updater-info-missing", NO_REPOSITORY_INFO_JSON, False, True, False),
         (
             "test-updater-invalid-snapshot-meta-field-missing",
             METADATA_FIELD_MISSING,
             False,
             True,
+            False,
         ),
     ],
 )
@@ -280,6 +295,7 @@ def test_updater_invalid_update(
     updater_repositories,
     client_dir,
     expect_partial_update,
+    should_last_validated_exist,
 ):
     repositories = updater_repositories[test_name]
     clients_auth_repo_path = client_dir / AUTH_REPO_REL_PATH
@@ -292,7 +308,9 @@ def test_updater_invalid_update(
         auth_repo_name_exists=auth_repo_name_exists,
     )
     # make sure that the last validated commit does not exist
-    _check_if_last_validated_commit_exists(clients_auth_repo_path)
+    _check_if_last_validated_commit_exists(
+        clients_auth_repo_path, should_last_validated_exist
+    )
 
 
 @pytest.mark.parametrize(
@@ -313,7 +331,7 @@ def test_valid_update_no_auth_repo_one_invalid_target_repo_exists(
         client_dir, repositories, expected_error, True
     )
     # make sure that the last validated commit does not exist
-    _check_if_last_validated_commit_exists(clients_auth_repo_path)
+    _check_if_last_validated_commit_exists(clients_auth_repo_path, True)
 
 
 def test_updater_expired_metadata(updater_repositories, origin_dir, client_dir):
@@ -328,7 +346,7 @@ def test_updater_expired_metadata(updater_repositories, origin_dir, client_dir):
         client_dir, repositories, ROOT_EXPIRED, False, set_time=False, strict=True
     )
     # make sure that the last validated commit does not exist
-    _check_if_last_validated_commit_exists(clients_auth_repo_path)
+    _check_if_last_validated_commit_exists(clients_auth_repo_path, False)
 
 
 @pytest.mark.parametrize(
@@ -447,10 +465,16 @@ def _check_last_validated_commit(clients_auth_repo_path):
     assert head_sha == last_validated_commit
 
 
-def _check_if_last_validated_commit_exists(clients_auth_repo_path):
+def _check_if_last_validated_commit_exists(clients_auth_repo_path, should_exist):
     client_auth_repo = AuthenticationRepository(path=clients_auth_repo_path)
     last_validated_commit = client_auth_repo.last_validated_commit
-    assert last_validated_commit is None
+    if not should_exist:
+        assert last_validated_commit is None
+    else:
+        assert (
+            client_auth_repo.top_commit_of_branch(client_auth_repo.default_branch)
+            == last_validated_commit
+        )
 
 
 def _check_if_commits_match(

taf/updater/updater.py

@@ -5,7 +5,10 @@
 from logdecorator import log_on_error
 from taf.git import GitRepository
 from taf.updater.types.update import UpdateType
-from taf.updater.updater_pipeline import AuthenticationRepositoryUpdatePipeline
+from taf.updater.updater_pipeline import (
+    AuthenticationRepositoryUpdatePipeline,
+    _merge_commit,
+)
 
 from pathlib import Path
 from taf.log import taf_logger, disable_tuf_console_logging
@@ -449,10 +452,19 @@ def _update_named_repository(
         # use last validated commit - if the repository contains it
 
         # all repositories that can be updated will be updated
-        if not only_validate and len(commits) and update_status == Event.CHANGED:
+        if (
+            not only_validate
+            and len(commits)
+            and (update_status == Event.CHANGED or update_status == Event.PARTIAL)
+        ):
+            # when performing breadth-first update, validation might fail at some point
+            # but we want to update all repository up to it
+            # so set last validated commit to this last valid commit
             last_commit = commits[-1]
             # if there were no errors, merge the last validated authentication repository commit
-            _merge_commit(auth_repo, auth_repo.default_branch, last_commit, checkout)
+            _merge_commit(
+                auth_repo, auth_repo.default_branch, last_commit, checkout, True
+            )
             # update the last validated commit
             if not excluded_target_globs:
                 auth_repo.set_last_validated_commit(last_commit)
@@ -540,35 +552,6 @@ def _update_transient_data(
     return update_transient_data
 
 
-def _merge_commit(repository, branch, commit_to_merge, checkout=True):
-    """Merge the specified commit into the given branch and check out the branch.
-    If the repository cannot contain unauthenticated commits, check out the merged commit.
-    """
-    taf_logger.info("Merging commit {} into {}", commit_to_merge, repository.name)
-    try:
-        repository.checkout_branch(branch, raise_anyway=True)
-    except GitError as e:
-        # two scenarios:
-        # current git repository is in an inconsistent state:
-        # - .git/index.lock exists (git partial update got applied)
-        # should get addressed in https://github.com/openlawlibrary/taf/issues/210
-        # current git repository has uncommitted changes:
-        # we do not want taf to lose any repo data, so we do not reset the repository.
-        # for now, raise an update error and let the user manually reset the repository
-        taf_logger.error(
-            "Could not checkout branch {} during commit merge. Error {}", branch, e
-        )
-        raise UpdateFailedError(
-            f"Repository {repository.name} should contain only committed changes. \n"
-            + f"Please update the repository at {repository.path} manually and try again."
-        )
-
-    repository.merge_commit(commit_to_merge)
-    if checkout:
-        taf_logger.info("{}: checking out branch {}", repository.name, branch)
-        repository.checkout_branch(branch)
-
-
 @timed_run("Validating repository")
 def validate_repository(
     clients_auth_path,