Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

visibility should be set on create and removed on update #680

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions examples/repo_org_internal/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Repository Visibility with Org, type internal

This demos various repository [visibility settings](https://help.github.com/en/github/administering-a-repository/setting-repository-visibility) for repositories.

This example will create a repositories in the specified `owner` organization. See https://www.terraform.io/docs/providers/github/index.html for details on configuring [`providers.tf`](./providers.tf) accordingly.

Alternatively, you may use variables passed via command line:

```console
export GITHUB_OWNER=
export GITHUB_TOKEN=
```

```console
terraform apply \
-var "owner=${GITHUB_OWNER}" \
-var "github_token=${GITHUB_TOKEN}"
```
10 changes: 10 additions & 0 deletions examples/repo_org_internal/cli.cfg
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
provider_installation {
filesystem_mirror {
path = "/Users/jurgen.weber/checkouts/terraform/terraform-provider-github/examples/repo_org_internal/terraform.d/plugins"
include = ["*/*/*"]
}
direct {
exclude = ["*/*/*"]
}
}

Comment on lines +1 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
provider_installation {
filesystem_mirror {
path = "/Users/jurgen.weber/checkouts/terraform/terraform-provider-github/examples/repo_org_internal/terraform.d/plugins"
include = ["*/*/*"]
}
direct {
exclude = ["*/*/*"]
}
}
See Building The Provider instructions in CONTRIBUTING.md

Can we make this more general by pointing to docs in the repo?

5 changes: 5 additions & 0 deletions examples/repo_org_internal/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
resource github_repository internal {
name = "mtribes-jurgen"
description = "A internal-visible repository created by Terraform"
visibility = "internal"
}
4 changes: 4 additions & 0 deletions examples/repo_org_internal/outputs.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
output internal_repository {
description = "Example repository JSON blob"
value = github_repository.internal
}
4 changes: 4 additions & 0 deletions examples/repo_org_internal/providers.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
provider github {
owner = var.owner
token = var.github_token
}
9 changes: 9 additions & 0 deletions examples/repo_org_internal/variables.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
variable owner {
description = "GitHub owner used to configure the provider"
type = string
}

variable github_token {
description = "GitHub access token used to configure the provider"
type = string
}
16 changes: 9 additions & 7 deletions github/resource_github_repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -271,11 +271,6 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta interface{}) er
repoReq := resourceGithubRepositoryObject(d)
owner := meta.(*Owner).name

// Auth issues (403 You need admin access to the organization before adding a repository to it.)
// are encountered when the resources is created with the visibility parameter. As
// resourceGithubRepositoryUpdate is called immediately after, this is subsequently corrected.
repoReq.Visibility = nil

repoName := repoReq.GetName()
ctx := context.Background()

Expand Down Expand Up @@ -462,8 +457,15 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta interface{}) er

repoReq := resourceGithubRepositoryObject(d)

// The endpoint will throw an error if trying to PATCH with a visibility value that is the same
if !d.HasChange("visibility") {
if d.HasChange("visibility") {
// The endpoint will throw an error if this repo is being created and the old value is ""
o, n := d.GetChange("visibility")
log.Printf("[DEBUG] Old Value %v New Value %v", o, n)
if o.(string) == "" {
repoReq.Visibility = nil
}
} else {
// The endpoint will throw an error if trying to PATCH with a visibility value that is the same
Comment on lines +460 to +468
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if d.HasChange("visibility") {
// The endpoint will throw an error if this repo is being created and the old value is ""
o, n := d.GetChange("visibility")
log.Printf("[DEBUG] Old Value %v New Value %v", o, n)
if o.(string) == "" {
repoReq.Visibility = nil
}
} else {
// The endpoint will throw an error if trying to PATCH with a visibility value that is the same
// The endpoint will throw an error if trying to PATCH with a visibility value that is the same
if !d.HasChange("visibility") || d.IsNewResource() {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kfcampbell, if you're looking at this today, you might find this simplification useful.

repoReq.Visibility = nil
}

Expand Down