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

jurgenweber · 2021-01-29T03:38:38Z

You can find a lot of my journey in the above ticket and with the help of some at google/go-github and github support I have found my problem.

Having the visibility removed on create, meant I could not create internal repos (when I do not have permissions to create public repos in my org) and the comment I removed

        // 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.

is the exact issue and problem I was facing, but this is suggesting that it should solve it. Since you can not update the visibility of a repo (edit: I later learned that this is not true, you can edit the visibility of repos), leaving it to update will always error also, as per the other comment:

// The endpoint will throw an error if trying to PATCH with a visibility value that is the same

this patch will always fail, so it should never run.

this PR solves this repo create problem, by ensuring that Visibility is set on create and never updates.

I also added in the example/demo I was using to test my provider locally.

Thanks

Note; If there are circumstances in which a repo can change its visibility do we know what they all are and we can update this accordingly.

jurgenweber · 2021-02-01T03:51:10Z

I looked into this a bit more, it seems you can change the visibility of a repo, the problem is... On create a change from the TF SDK is computed, from "" to "internal". I added this test in.

The problem now is that even though this is created as visibility Internal and the update happens with no change.. The repo is public!? Seems like my org's ability to make public's repos changed over the week also. :)

jurgenweber · 2021-02-02T22:24:25Z

All in all, this requires a bit of testing to handle all of the scenarios.

Do we have access to a private user, an org with the ability to create public repos, an org with out the ability to create public repos.

Thanks

jcudit · 2021-02-03T21:24:50Z

Spent some time with this branch and tested a few of the scenarios mentioned. The experience was quite buggy so I'd like to defer this work to batch it in with related improvements to the repository resource. I believe this is an important problem to fix, but want to ship a few outstanding PRs before coming back to this one.

jurgen-weber-deltatre · 2021-02-03T21:58:32Z

Ok, how did you test them? What were the bugs. I have some capacity so I am happy to continue with it and get it to where it is needed.

jcudit · 2021-02-05T14:04:08Z

I was working out of the example in this PR but kept hitting errors when deleting resources. I attempted this against a GHES instance using a token with full permissions to a terraformtesting organization. Also tried to create a user repository instead of an organization repository. It seemed to be getting tripped up on disabling vulnerability alerts 😕 . Have you seen this behaviour?

$ terraform apply -auto-approve   -var "owner=${GITHUB_OWNER}"   -var "github_token=${GITHUB_TOKEN}"
github_repository.internal: Creating...

Error: DELETE https://<host>/api/v3/repos/terraformtesting/mtribes-jurgen/vulnerability-alerts: 404 Not Found []

  on main.tf line 1, in resource "github_repository" "internal":
   1: resource github_repository internal {

I hit my time limit while troubleshooting, but aim to pick this back up now that v4.4.0 is shipped. Was planning to knock out a couple of vulnerability related bugs to better understand that code path before addressing whatever the bug is here that is blocking the example from running for me.

jurgenweber · 2021-02-05T20:32:32Z

odd, I had no issues with any repo features, just the create process.

lneuhaus · 2021-03-01T13:49:34Z

Is someone still working on this? Can I help, e.g. by testing and reporting? One of my projects is blocked by this fix getting merged.

jcudit · 2021-03-08T16:26:18Z

A few of us are going to pair on reviews this week. Aiming to get this one tested out during the session and return here with next steps. 🤞🏾 that things just work and we can get to shipping.

jcudit

Gave this another pass and think we should get some tests around repository visibility included with this improvement. Seems like something that deserves more confidence as we make changes in this area. d086915 is a start that can be cherry-picked. Is there any interest in picking things up from here?

jcudit · 2021-03-12T21:35:30Z

examples/repo_org_internal/cli.cfg

+provider_installation {
+  filesystem_mirror {
+    path    = "/Users/jurgen.weber/checkouts/terraform/terraform-provider-github/examples/repo_org_internal/terraform.d/plugins"
+    include = ["*/*/*"]
+  }
+  direct {
+    exclude = ["*/*/*"]
+  }
+}
+


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?

jurgenweber · 2021-03-15T00:08:58Z

Yeah, up to you guys. I will be honest and say. I don't have the space for this at the moment.. So feel free to butcher close and carry on with this PR as you see fit.

kfcampbell · 2021-03-15T16:27:31Z

I'm taking a few days off later in the week so I'll poke at this a bit and start by cherry-picking @jcudit's test case. I'll post an update here in a few days!

tibbes · 2021-03-17T10:36:17Z

github/resource_github_repository.go

+	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


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() {

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

jcudit · 2021-04-09T11:42:35Z

Superseded by #746.

Huge thanks again @jurgen-weber-deltatre for getting this effort kicked off ❤️ . Hoping this tests well within the community as it has been a long-standing pain point for the project.

jurgenweber · 2021-04-09T23:30:32Z

no problems. Glad to hear you got somewhere.

jurgenweber added 2 commits January 29, 2021 14:33

visibility is required on create and can not be updated

55ba811

add an example

cbb9b2d

jurgenweber mentioned this pull request Jan 29, 2021

fix api preview header, missing : google/go-github#1786

Closed

This was referenced Jan 29, 2021

Cannot create repository with 'visibility: private' in an organisation that does not allow public repositories #669

Closed

visibility = "private" creates public repo first and then changes it to private #580

Closed

update logic for testing if there is an adequate change

6754aed

jcudit added this to the v4.3.3 milestone Feb 1, 2021

jcudit modified the milestones: v4.3.3, v4.5.0 Feb 3, 2021

jcudit modified the milestones: v4.7.0, v4.6.0 Mar 8, 2021

jcudit reviewed Mar 14, 2021

View reviewed changes

tibbes reviewed Mar 17, 2021

View reviewed changes

jcudit modified the milestones: v4.6.0, v4.7.0 Mar 23, 2021

kfcampbell mentioned this pull request Apr 3, 2021

Visibility lifecycle fixes #746

Merged

jcudit closed this Apr 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jurgenweber commented Jan 29, 2021 •

edited

Loading

jurgenweber commented Feb 1, 2021 •

edited

Loading

jurgenweber commented Feb 2, 2021

jcudit commented Feb 3, 2021

jurgen-weber-deltatre commented Feb 3, 2021

jcudit commented Feb 5, 2021

jurgenweber commented Feb 5, 2021

lneuhaus commented Mar 1, 2021

jcudit commented Mar 8, 2021

jcudit left a comment

jcudit Mar 12, 2021

jurgenweber commented Mar 15, 2021

kfcampbell commented Mar 15, 2021

tibbes Mar 17, 2021

tibbes Mar 17, 2021

jcudit commented Apr 9, 2021

jurgenweber commented Apr 9, 2021

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

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

Conversation

jurgenweber commented Jan 29, 2021 • edited Loading

jurgenweber commented Feb 1, 2021 • edited Loading

jurgenweber commented Feb 2, 2021

jcudit commented Feb 3, 2021

jurgen-weber-deltatre commented Feb 3, 2021

jcudit commented Feb 5, 2021

jurgenweber commented Feb 5, 2021

lneuhaus commented Mar 1, 2021

jcudit commented Mar 8, 2021

jcudit left a comment

Choose a reason for hiding this comment

jcudit Mar 12, 2021

Choose a reason for hiding this comment

jurgenweber commented Mar 15, 2021

kfcampbell commented Mar 15, 2021

tibbes Mar 17, 2021

Choose a reason for hiding this comment

tibbes Mar 17, 2021

Choose a reason for hiding this comment

jcudit commented Apr 9, 2021

jurgenweber commented Apr 9, 2021

jurgenweber commented Jan 29, 2021 •

edited

Loading

jurgenweber commented Feb 1, 2021 •

edited

Loading