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

Add visibility parameter to data source and resource #454

Merged
merged 2 commits into from
Jun 25, 2020

Conversation

patrickmarabeas
Copy link
Contributor

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

@ghost ghost added size/XS Type: Documentation Improvements or additions to documentation labels May 11, 2020
@patrickmarabeas patrickmarabeas mentioned this pull request May 11, 2020
@anGie44 anGie44 added the Type: Feature New feature or request label May 11, 2020
@anGie44 anGie44 self-assigned this May 12, 2020
@patrickmarabeas
Copy link
Contributor Author

Definitely something funky going on.

@@ -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)),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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())
Copy link
Contributor

@anGie44 anGie44 May 12, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@anGie44
Copy link
Contributor

anGie44 commented May 12, 2020

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

@patrickmarabeas
Copy link
Contributor Author

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
Copy link
Contributor

anGie44 commented May 12, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor

anGie44 commented May 12, 2020

I'll try d.HasChange.

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

@ghost ghost added size/S and removed size/XS labels May 12, 2020
@patrickmarabeas
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@anGie44 anGie44 requested a review from jcudit May 12, 2020 14:28
@patrickmarabeas
Copy link
Contributor Author

patrickmarabeas commented May 13, 2020

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
Copy link
Contributor

anGie44 commented May 13, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@anGie44 anGie44 added this to the v2.8.1 milestone May 15, 2020
Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@ghost ghost added size/M and removed size/S labels May 25, 2020
@patrickmarabeas
Copy link
Contributor Author

patrickmarabeas commented May 25, 2020

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
Copy link
Contributor

jcudit commented May 25, 2020

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
Copy link
Contributor Author

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

@jcudit
Copy link
Contributor

jcudit commented May 26, 2020

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
Copy link
Contributor

jcudit commented May 31, 2020

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

@jcudit jcudit modified the milestones: v2.9.0, v2.10.0 Jun 3, 2020
@patrickmarabeas
Copy link
Contributor Author

@jcudit commits cherry-picked and PR rebased

@jcudit
Copy link
Contributor

jcudit commented Jun 24, 2020

@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
Copy link
Contributor Author

@jcudit rebased.

@jcudit jcudit merged commit 3a52bc6 into integrations:master Jun 25, 2020
@jcudit jcudit modified the milestones: v2.10.0, v2.9.0 Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Type: Documentation Improvements or additions to documentation Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support "internal" repository types
3 participants