Instanciate new empty lists for each instance of WorkflowRepoManager

[mgoerens]

Previously we were assigning values to attributes on the class level. For list in particular, this had the side effect of leaking values between instances:

>>> m = OwnersFileSubmissionsE2ETest()
>>> m.repo_manager._WorkflowRepoManager__local_branches_created
['e2e-owners-d376dfa-6d3820eb5c624ddc9066f96f1a69640e']
>>> m.cleanup()
>>> m.repo_manager._WorkflowRepoManager__local_branches_created
[]
>>> m = OwnersFileSubmissionsE2ETest()
>>> m.repo_manager._WorkflowRepoManager__local_branches_created
['e2e-owners-d376dfa-6d3820eb5c624ddc9066f96f1a69640e', 'e2e-owners-d376dfa-cc7f8acf1850498cb6c758bc286f2bec']

Using a dataclass for WorkflowRepoManager, we can ensure that empty lists are initialized for each new instance, and attributes values remain independant.

Fix #414

[komish]

As you know, I'm not a particularly huge fan of dataclasses in class definitions that have a substantial amount of logic. With that said, I think this may solve the problem, but I still feel it changes the contract of the class in a way that may not be strictly required.

The issue with WorkflowRepoManager causing this bug is that I heavily used class attributes instead of instance attributes. I did some reading, and this is bug seems to be caused strictly by assumptions I made in how attributes should be defined.

I think the following solves this in the exact same way, without the use of fields or dataclasses. We're not using any additional features of fields and dataclasses, so I'd advocate against using them here.

class WorkflowRepoManager:
    def __init__(self):
        logging.debug(f"{self} --> __init__ called!")

        # Keep a log of things created so we can clean them up.
        self.__local_branches_created: list[str] = []
        self.__local_worktrees_created: list[TemporaryDirectory] = []
        # (remote, branch), e.g. ('openshift-helm-charts/charts, 'my-pr-branch')
        self.__remote_branches_created: list[tuple[str, str]] = []

        # The token to use for GitHub API operations.
        self.__authtoken: str = ""

        # Working directory at instantiation.
        self.old_cwd: str = os.getcwd()

        # The repository at working directory.
        try:
            self.repo = git.Repo()
        except git.InvalidGitRepositoryError as e:
            raise RepoManagementError(
                "Unable to initialize git repository. Is the current directory a git repo?"
            ) from e

        # The branch at repo initialization. On Cleanup, we return to this branch
        # before we remove locally generated branches.
        try:
            self.original_branch = self.repo.active_branch.name
        except TypeError:
            self.original_branch = self.repo.git.rev_parse("--short", "HEAD")
...

In other words, if I had just instantiated all of the variables in __init__(...), we probably wouldn't have this buggy behavior. My apologies.

Let me know what you think.

[komish]

Suggested change

	__remote_branches_created: list[(str, str)] = field(default_factory=list)
	__remote_branches_created: list[tuple[str, str]] = field(default_factory=list)