Issue when default branch has been changed and you try to delete a file

[Waterdrips]

We changed the default branch on a repository, and we use the branch name as part of the filenames.

The changing of the default branch caused these two changes:

• +- the file (due to name change)
• +- the default branch

The branch got changed first which means the below code snipped grabbed the commit SHA from branchA which is referenced by the file's branch in state. Then the next step gets the DEFAULT BRANCH (branchB) from the repo's matching commit, and then applies a DELETE of the file only. This however means that the default branch will be reverted to the head commit of the branch referenced in the File's state (our case, branchA). This is then Commited and merged into the default branch, branchB. The PR therefore removes any changes made to branchB since it was last in-sync with branchA.

terraform-provider-githubfile/githubfile/resource_file.go

Lines 131 to 148 in 99b9540

	// Get the tree that corresponds to the target branch.
	s, err := branch.GetSHAForBranch(context.Background(), c.githubClient, f.repositoryOwner, f.repositoryName, f.branch)
	if err != nil {
	return err
	}
	oldTree, _, err := c.githubClient.Git.GetTree(context.Background(), f.repositoryOwner, f.repositoryName, s, true)
	if err != nil {
	return err
	}
	// Remove the target file from the list of entries for the new tree.
	// NOTE: Entries of type "tree" must be removed as well, otherwise deletion won't take place.
	newTree := make([]github.TreeEntry, 0, len(oldTree.Entries))
	for _, entry := range oldTree.Entries {
	if *entry.Type != "tree" && *entry.Path != f.path {
	newTree = append(newTree, entry)
	}
	}

I'm not sure if we can work out if we should apply the change to the file's branch and then PR that change into the default branch? Possibly change

terraform-provider-githubfile/githubfile/resource_file.go

Line 136 in 99b9540

oldTree, _, err := c.githubClient.Git.GetTree(context.Background(), f.repositoryOwner, f.repositoryName, s, true)

This caused the provider to silently remove some extra code from the newly default branch.

[frncmx]

terraform-provider-githubfile/githubfile/resource_file.go

Line 132 in 99b9540

s, err := branch.GetSHAForBranch(context.Background(), c.githubClient, f.repositoryOwner, f.repositoryName, f.branch)

f.branch above should be the current default branch instead I believe.
Later when we transform the tree we only need f.path.
However, since we transform f.branch's tree that can be anything and we just push that to the default branch currently.

Later, CreateCommit is going to make the PR against the default branch, since the branch option is not provided.
Note: I would also explicitly provide CommitOptions.Branch to CreateCommit.
The default branch can be obtained also as in the below snippet.

https://github.com/form3tech-oss/go-github-utils/blob/master/pkg/commit/commit.go#L45-L47

Disclaimer: I still don't like that we transform such a low-level and high-risk object (git-tree). At least we should make a diff check somehow.

[Waterdrips]

See Im not 100% sure on this solution, it did cross my mind yesterday.

If we think about what this provider is "promising to do" when we create a resource

resource "githubfile_file" "file" {
  repository_owner = "me"
  repository_name  = "example"
  branch           = "test"
  path             = "/some/path/file.txt"
  contents         = "File Contents!!"
}

We are saying we will track a file on a specific branch, so the delete operation in the provider should delete the file in that branch.

If ,as in our case, we are changing the branch then we need the branch change to force a new resource and the delete to honor the branch in the config. When we change it, the delete will be oldBranch and then we create a new file on newBranch

[frncmx]

I think that makes sense. I've one concern though. If that branch where we destroy the resource is going to be branch=master then there is no room for mistakes. Since the actual change (git diff) does not go through code review, it commits straight into master. If master has auto-deploy then that is an unattended change deployed straight into prod.

On the other hand, I agree with your proposal. Otherwise, the resource cannot converge, e.g., from the above snippet:

• Given that branch=test and the default branch is develop
• TF adds the file to develop but it actually wants it in test
• No convergence

[frncmx]

• Would it be feasible to show the actual git diff patch in the TF plan?
• Between the plan and apply, do we store the generated patch, and do we apply that?

[Waterdrips]

If that branch where we destroy the resource is going to be branch=master then there is no room for mistakes.

Currently if a user has branch=master then it commits directly to there at the moment, this doesn't change that

• Would it be feasible to show the actual git diff patch in the TF plan?

• Between the plan and apply, do we store the generated patch, and do we apply that?
  TF would only know that the resource needed to change, it wouldn't be able to show the actual git patch patch because we couldn't manage the state on a patch in the statefile so theres no real way to get the output displayed even if we calculated it