Changing auto init value should not force a repo to be destroy/created

[majormoses]

Terraform Version

$ terraform --version
Terraform v0.11.7

Your version of Terraform is out of date! The latest version
is 0.11.8. You can update by downloading from www.terraform.io/downloads.html

Plugin version 1.3

Affected Resource(s)

Please list the resources as a list, for example:

• github_repository

If this issue appears to affect multiple resources, it may be an issue with Terraform's core, so please mention this.

Terraform Configuration Files

resource "github_repository" "example" {
  name        = "example"
  description = "My awesome codebase"
  auto_init = false

  private = true
}

resource "github_repository" "example" {
  name        = "example"
  description = "My awesome codebase"
  auto_init = true

  private = true
}

Expected Behavior

auto_init: (Optional) Meaningful only during create; set to true to produce an initial commit in the repository.

To me this should mean that changes to this should ignore any changes. I found in my org that it was something that we used to also toggle branch protection count.

It should ideally note that there was a change but not actually send that request as it does not make sense that someone accidentally or intentionally flipping the value would blow up their code base.

Actual Behavior

marks the resource for deletion/creation

Steps to Reproduce

Please list the steps required to reproduce the issue, for example:

1. terraform plan
2. terraform apply
3. make changes to flip auto_init value
4. terraform plan

References

• https://github.com/sous-chefs/terraform-github-org/pull/111
• https://github.com/terraform-providers/terraform-provider-github/issues/148
• https://github.com/terraform-providers/terraform-provider-github/pull/135

[salvianreynaldi]

I'm also experiencing this issue. I think it was not like this in the previous provider version
I just checked that this is not the case in provider version 1.1.0
with this issue it's basically impossible to import repositories

[majormoses]

@salvianreynaldi so for imports there is an unreleased fix that has been merged but I think in general this is still a problem. Per the docs its only meaningful during creation so we should ignore it rather than force a new resource. In my use case I am using auto_init to determine if we can enable branch protections, while I could use another value to control this I think it defies the users expectations. It minimally needs a documentation update.

[majormoses]

@radeksimko any thoughts? I'd really like to see this reverted to the old behavior.

[radeksimko]

Hi folks,
import was indeed fixed in #154, but for other use cases I believe the resource behaves as intended.

In my use case I am using auto_init to determine if we can enable branch protections

Can you explain your use case a bit more? I'm not sure I fully understand the context there.

Thanks.

[majormoses]

Sorry been traveling for work. So our use case is that sometimes you want to create a repository without initialization (maybe to copy the history from a public project to a private one and is not a normal fork scenerio) we have a module that used auto_init to determine the count of the branch protection since you can't create a branch protection for a branch that does not exist (at least in the v3 api). After they have done whatever custom workflow is required to the get repository into the right sate they would flip auto_init to true which would then do the branch protection. The advantage of piggybacking off of this is that it requires only changing one variable in each scenario where now with this change we would need to introduce two variables that do essentially the same thing. Let me know if its unclear and I can share some snippets from our internal modules (after scrubbing some of the more business logic that would not apply to anyone else. In 0.12 we will be able to set null variables so we could leverage that when it comes but if you ask me the change that was made is a breaking one without any particular value I can't see anyone actually wanting to blow up their repo to change the license/gitignore template or auto init, these only make sense to evaluate in the context of a new resource and having someone who actually wants this can run a taint on the resource and get the desired behavior.

[majormoses]

Also that has not been released so I am in a broken state for the moment. Can we please revert the change and discuss how to do this without breaking people?

[majormoses]

In the mean time I have had to add:

lifecycle {
    ignore_changes = ["auto_init"]
  }

[majormoses]

@radeksimko let me know if you need any further clarification

Here is a snippet from our module illustrating what we are doing a bit more:

resource "github_repository" "repo" {
  name               = "${var.repo_name}"
  description        = "${var.repo_description}"
  archived           = "${var.repo_archived}"
  has_downloads      = true
  has_issues         = true
  has_wiki           = true
  homepage_url       = "${var.repo_homepage_url}"
  private            = "${var.private_repo}"
  allow_merge_commit = "${var.allow_merge_commit}"
  allow_squash_merge = "${var.allow_squash_merge}"
  allow_rebase_merge = "${var.allow_rebase_merge}"
  has_projects       = false

  # this automatically creates a commit and pushes to master and is needed
  # because your can't add branch protection for an uninitialized repo
  # since the branch will not exist yet. It is only relevant during repo
  # creation from github api side but we also use it to determine if branch
  # protections should be enabled.
  auto_init = "${var.auto_init}"

  # default_branch = "${var.repo_default_branch}"

  topics = "${var.repo_topics}"

  # temp for migration purposes only
  # lifecycle {
  #   ignore_changes = ["name", "id", "repository"]
  # }

  # this is a terrible hack that is needed because of
  # https://github.com/terraform-providers/terraform-provider-github/issues/155
  # NOTE: that we only do this on the repo and still care about it when looking
  # at branch protection
  lifecycle {
    ignore_changes = ["auto_init"]
  }
}

# removed a bunch of irrelevant config

# setup branch protection
resource "github_branch_protection" "master" {
  # this is a bit of a hack to allow people to create repositories uninitialized
  # and then add branch protection later. The use cases are mostly around needing
  # to create "forked" private repositories
  count = "${var.auto_init}"

  depends_on = [
    "github_repository.repo",
    "github_team_repository.reviewers",
    "github_team_repository.admins",
  ]

  repository = "${var.repo_name}"
  branch     = "master"

  # defaults to true
  enforce_admins = "${var.enforce_admins}"

  required_status_checks {
    # this really should be enabled whereever possible
    strict = "${var.require_pull_request_updated_branch}"

    # might want to expose this as a var later
    contexts = "${var.required_ci_contexts}"
  }

  required_pull_request_reviews {
    dismiss_stale_reviews = "${var.dismiss_stale_reviews}"

    # dismissal_users = ["foo-user"]
    dismissal_teams = ["${var.dismissal_teams}"]

    # intentionally not made a variable
    require_code_owner_reviews = true
  }

  restrictions {
    # this should not be used lightly but is an option to allow some "power users"
    # additional access without giving them everything. This should most commonly
    # done on a per repo basis but thats just me :shrug:
    users = "${var.users_that_can_merge}"

    # these are names rather than ids
    teams = [
      "${var.reviewer_teams}",
    ]
  }
}

# removed a bunch of irrelevant config

We basically keyed branch protection setup off auto_init value so it could be flipped as a single variable rather than requiring two variables to essentially be the same. Github recently added support for creating branch protections without branches needing to exist so this may not be an issue anymore for my use case but I still think this subverts everyones expectations based on real world usage and what the documentation says. Even if we decide to keep this we should not be making breaking changes like this should be called out in the changelog as breaking and ideally should only be done in a major version bump so that people can use pessimistic version constraints safely.

[majormoses]

when this is added: https://github.com/terraform-providers/terraform-provider-github/issues/167 I wn't need the hack but I still question why this change made sense.

[estahn]

I had to disable auto_init to import our repositories. After the import I tried to add it back into our module but that would then start destroying repositories. Why is auto_init relevant for anything else than repository creation?

[sudoforge]

This is still an issue, with provider v1.3.

I am unable to import repositories that were created manually, outside of Terraform. When a repository is created manually, the auto_init property is an empty string (assuming that the user doesn't manually select "create an initial commit"). This means that adopting this provider is effectively impossible without recreating all of the repositories that are created manually.

[majormoses]

@sudoforge in the mean time you can import and it should work with 2.0.0 which contains a partial fix.

I am able in my case also use an ignore_changes = ["auto_init"] so effectively we can do whats needed. At this point for me its only a problem because I used it for more than what auto_init meant, the default behavior, and that according to the documentation once it is set it is ignored by the backend if it already exists. We decided based on this to only ask our users for one input instead of two, this has backfired; this can be fixed with some refactoring, requiring additional parameters, and state manipulation. That was all done under the assumption that there is no good reason to propose deleting a resource because of a resource according to their api docs indicate that this value you can send whatever you want and it will be ignored on any request other than the creation. Because of the way terraform works it sees a change on an immutable object (which the old behavior it was not) and therefore says you should recreate. This makes sense and the behavior internally at github also likely matches this but as an external component it gets kinda awkward past initial creation. Perhaps if it did a query to see if the repo already exists it can omit the property as github would discard / ignore it anyways. Essentially this would all be fixed by simply not marking this object mutable as from GitHub's perspective. If that is the case just taint the module and it accomplishes the same thing.

This needlessly broke users. I would like to propose we only mark parameter objects as immutable in a major release to avoid breaking users pinning via pessimistic version constraints and follow semver to ensure that all changes come in are safe for new and imported resources. Doing so would prevent further issues and forces an operator to make an intentional choice (or lack of pinning) to destroy a resource out of a tainted state object itself.

[majormoses]

@radeksimko @paultyng @tracypholmes any update/feedback? I would like to make a PR to fix this but wanted to see if it would be accepted beforehand since there was some controversy around this.

[tylersmith34]

Please fix this, I just ran through this fire drill on 200+ repos and had to use the ignore_changes hack.

[majormoses]

Hey all: I have proposed rolling back the changes in #317 but it's up to the maintainers to accept it. So far @radeksimko has been against it without providing any further rationale so I can't really say if this will be accepted or even entertained.

[martinb3]

FWIW, the same problem exists for setting a template on an imported repo. While only relevant during creation, having a template on an imported repo causes the provider to want to re-create the repository based on the template.

[majormoses]

@martinb3 no guarantee that the maintainers (though I have much higher confidence with some newer maintainers) will merge it, but I opened #609 to address the same issue with the params outlined, let me know if I missed any.