Allow default branch to be set to master/main on update, using a new github_branch_default resource

[liamg]

Closes #193

Best explanation/reasoning for the change is here: https://github.com/terraform-providers/terraform-provider-github/pull/194#issuecomment-675587125

[liamg]

@radeksimko Apologies for the ping, but this is ready to be merged and fixes a bug that is hurting us - does it look good to you?

[majormoses]

Has something changed with the api or behavior from github? Based on my own experiences and the comments the reason its in place is because if you set the default branch and use auto_init = false[1] this will break. I agree that its probably not the right condition to check but I don't think this is a safe change. I would suggest we avoid setting it when auto_init = false as this covers the scenario better I think with fewer side effects.

[1]: a common scenario for this is to create an internal fork of a public project where you may need to diverge from the upstream.

[liamg]

This is true, though only on creation. You may want to update a repo that was initially created with auto_init set to false, but that now has a valid ref for the default branch.

I guess the only way to do this safely would be to check if the ref actually exists on the repo and fail early if not.

WDYT?

[liamg]

The specific check for master isn't really correct, because setting the default to any ref that doesn't exist will result in the same error.

[majormoses]

I agree the check for master is not really correct, that being said we need a condition to make it work for initial setup as well as future setups. I would say that if the ref does not exist and branch is master we set it to null and otherwise we set it to what was passed? I suppose we could also have a third condition of if its not master and ref does not exist we can error out there. I think this should cover both workflows.

[jtsaito]

Can I expect the suggested changes to be applied anytime soon?

[jcudit]

I'd like to help here but am unsure if I have understood the requested fix. Does this implementation satisfy the two use cases?

type validDefaultBranchOptions struct {
	d       *schema.ResourceData
	owner   string
	repo    string
	repoReq *github.Repository
	client  *github.Client
}

func validDefaultBranch(options validDefaultBranchOptions) (string, error) {

	// A `default_branch` is valid if it is safe for initial repository setup
	// as well as future setups

	defaultBranch := ""
	if v, ok := options.d.GetOk("default_branch"); ok {
		defaultBranch = v.(string)
	}

	// Assert requested default branch exists
	ctx := context.WithValue(context.Background(), ctxId, options.d.Id())
	_, _, err := options.client.Repositories.GetBranch(ctx, options.owner, options.repo, defaultBranch)
	if err != nil {
		// TODO: Match a specific error
		return defaultBranch, errors.New("default branch does not exist")
	}

	// Assert `auto_init` is not `false` when a valid default branch is requested
	if *options.repoReq.AutoInit == false {
		// When creating an internal fork of a public project,
		// setting the default branch alongside `auto_init = false` fails
		return defaultBranch, errors.New("unsupported repository for default branch request")
	}

	return defaultBranch, nil
}

@liamg @majormoses - does checking for branch existence and refusing when auto_init is false address all concerns without raising new ones?

[majormoses]

@jcudit thanks for taking a look I am not too familiar with go but it looks right. It's been a long time so I am trying to remember back to the problem so apologies if I get some details wrong. The problem as I understand it is how to support:

• creating a new repository: can fail because it does not wait for the ref to exist.
• importing a repository and modifying it after its existence: With an import it implicitly means that the resource existed so we don't need any special handling we just do what is requested.
• updating a resource in a state: In the cause of created or imported resources that are later updated there would have to be some branch regardless of its name. It could fail if someone tries to set it to a non existing branch, it would have to be a branch and not a generic ref (sha, tag) which we do in your proposed patch.

I believe the reason the check for master was for initial creation as the requested resource did not exist github would error out. At the time terraform did not have the concept of an exposed null input/variable type (until 0.12+) making that check for "master" something that solved the issue. Perhaps this might be a better to switch this check to null as its intent will be clearer and its behavior can entirely be controlled by the caller. Setting this to null would mean that we do not manage this setting until it is changed to a non null value and in a way is similar to ignore_changes experience. Defaulting to null will keep the existing behavior with solving our issues. The workflow for creating a new repo with a non default branch would be:

1. create repo (omit or explicitly set to null)
2. push/create new branch
3. update terraform to change the default, terraform will ignore any changes to default branch unless you explicitly ask for it

I am not sure if I thought through this but the idea of terraform checking out some other repo feels odd. I view the Terraform provider responsibility in the context of github is to manage github as a service not to actually interact with git directly. It seems harmless enough but I am not sure where blurring those lines could lead to.

[shogo82148]

ping
I have same issue with the latest releases (Terraform v0.13.0 and GitHub provider v2.9.2).
any progress?

[jacobeatsspam]

@jcudit @liamg I think I've caught up on all the comments, so here is my $0.02:

• default_branch is supposed to only be managed on PATCH, which means it shouldn't be sent on the first GET. Therefor, the check should be based on whether the repo already exists or not.

• This creates a fundamental workflow issue: One must define the repo, apply, update to point at a new branch, apply again. This creates a dependency cycle if the new branch is created with github_branch.

My suggestion is to create a new resource, github_branch_default, to track this attribute, and stop managing it in github_repository.

[majormoses]

• default_branch is supposed to only be managed on PATCH, which means it shouldn't be sent on the first GET. Therefor, the check should be based on whether the repo already exists or not.

• This creates a fundamental workflow issue: One must define the repo, apply, update to point at a new branch, apply again. This creates a dependency cycle if the new branch is created with github_branch.

My suggestion is to create a new resource, github_branch_default, to track this attribute, and stop managing it in github_repository.

Good find, I did not notice the patch vs get here. This actually makes a lot of sense now why I solved those problems within my module by abusing auto_init to create a workflow that does not have this issue but relies on 2 plan/apply cycles. Even with this removed I will likely need to keep those "hacks" as there are similar but different issues with branch protection. There is still a part of me that dislikes forcing it as another resource for a number of reasons that mostly boil down to user experience; mostly manifesting in migration/upgrade and breaking changes. I would think we may want to keep the current behavior (with deprecation warnings) and add the new resource this will help consumers identify the issues and how to resolve them.

[liamg]

@majormoses @jacobisaliveandwell Thanks for the discussion! I've run with the idea above and added a github_branch_default resource. This is my first full new resource so all criticism/ideas will be gratefully received! Hopefully my documentation change is also sufficient...

[liamg]

This could potentially help with the situation in #513 too

[majormoses]

LGTM

[majormoses]

LGTM

[jacobeatsspam]

Looks good!

[jacobeatsspam]

@jcudit this is ready for review

[jcudit]

This diff looks great, thanks for putting it together @liamg 🙇

[dwilliams782]

Any idea on timelines for this to be released? This would be extremely useful to restore "master" on new repositories given the recent changes...

[jcudit]

@dwilliams782 thanks for the ping. Lets get this into the next release.

@liamg are you around to address the outstanding conflict?

[liamg]

@jcudit Yep, conflicts resolved :)