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

create repository with internal visibility initially instead of changing it later #794

Merged

Conversation

jtgrohn
Copy link
Contributor

@jtgrohn jtgrohn commented May 18, 2021

Addresses #788

Tested by:

  • running the tests in github/resource_github_repository_test.go and
  • manually testing by creating an internal repository with member credentials in an organization that does not allow members to change repository visibility

Comment on lines -312 to -316
if isPrivate {
repoReq.Visibility = github.String("private")
} else {
repoReq.Visibility = github.String("public")
}
Copy link
Contributor Author

@jtgrohn jtgrohn May 18, 2021

Choose a reason for hiding this comment

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

This block was incorrectly overriding the Visibility property which was already set on line 262 by the call to resourceGithubRepositoryObject on line 287

@dininski
Copy link

Thanks for this! We are also affected by this and hopefully it can be merged fairly quickly.

@jcudit jcudit modified the milestones: v4.10.0, v4.11.0 May 22, 2021
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.

This is on the right track and likely can ship as-is. I'm having trouble verifying it against my test org due to unrelated issues that I'm hoping to clear early next week.

Here is the error I'm getting when running tests against an org that is incompatible with internal repositories:

---[ REQUEST ]---------------------------------------
POST /orgs/terraformtesting/repos HTTP/1.1
Host: api.github.com
User-Agent: go-github
Content-Length: 369
Accept: application/vnd.github.baptiste-preview+json, application/vnd.github.nebula-preview+json
Content-Type: application/json
Accept-Encoding: gzip

{
 "name": "tf-acc-test-visibility-internal-ianl1",
 "description": "",
 "homepage": "",
 "private": true,
 "visibility": "internal",
 "has_issues": false,
 "has_projects": false,
 "has_wiki": false,
 "is_template": false,
 "auto_init": false,
 "gitignore_template": "",
 "license_template": "",
 "allow_squash_merge": true,
 "allow_merge_commit": true,
 "allow_rebase_merge": true,
 "delete_branch_on_merge": false
}

-----------------------------------------------------
2021/05/21 21:06:48 [DEBUG] Github API Response Details:
---[ RESPONSE ]--------------------------------------
HTTP/2.0 500 Internal Server Error
Content-Length: 0
Access-Control-Allow-Origin: *
Access-Control-Expose-Headers: ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, Deprecation, Sunset
Content-Security-Policy: default-src 'none'
Content-Type: application/json; charset=utf-8
Date: Sat, 22 May 2021 01:06:47 GMT
Referrer-Policy: origin-when-cross-origin, strict-origin-when-cross-origin
Server: GitHub.com
Strict-Transport-Security: max-age=31536000; includeSubdomains; preload
Vary: Accept-Encoding, Accept, X-Requested-With
X-Accepted-Oauth-Scopes: public_repo, repo
X-Content-Type-Options: nosniff
X-Frame-Options: deny
X-Github-Media-Type: github.baptiste-preview; format=json, github.nebula-preview; format=json
X-Github-Request-Id: F5E9:31EF:1132938:2AFD4EB:60A85927
X-Oauth-Scopes: admin:gpg_key, admin:org, admin:org_hook, admin:public_key, admin:repo_hook, delete_repo, repo, user, workflow
X-Ratelimit-Limit: 5000
X-Ratelimit-Remaining: 363
X-Ratelimit-Reset: 1621645686
X-Ratelimit-Resource: core
X-Ratelimit-Used: 4637
X-Xss-Protection: 0


-----------------------------------------------------
2021/05/21 21:06:48 [DEBUG] github_repository.internal: apply errored, but we're indicating that via the Error pointer rather than returning it: POST https://api.github.com/orgs/terraformtesting/repos: 500  []
2021/05/21 21:06:48 [ERROR] <root>: eval: *terraform.EvalApplyPost, err: POST https://api.github.com/orgs/terraformtesting/repos: 500  []
2021/05/21 21:06:48 [ERROR] <root>: eval: *terraform.EvalSequence, err: POST https://api.github.com/orgs/terraformtesting/repos: 500  []
2021/05/21 21:06:48 [DEBUG] New state was assigned lineage "799979e3-4197-d83a-5f49-8869661a2f8e"
    testing.go:654: Step 0 error: errors during apply:
        
        Error: POST https://api.github.com/orgs/terraformtesting/repos: 500  []
        
          on /var/folders/t1/p9cyhv6s2wg4g8s_x6zkpz_m0000gn/T/tf-test287623353/main.tf line 2:
          (source code not available)

We may be able to expect this error or skip the test if we detect an org does not support internal repositories.

@jtgrohn
Copy link
Contributor Author

jtgrohn commented Jun 1, 2021

This is on the right track and likely can ship as-is. I'm having trouble verifying it against my test org due to unrelated issues that I'm hoping to clear early next week.

Here is the error I'm getting when running tests against an org that is incompatible with internal repositories:

We may be able to expect this error or skip the test if we detect an org does not support internal repositories.

Is there anything I can do to help? Would you like me to look into the internal incompatible orgs error?

@jcudit
Copy link
Contributor

jcudit commented Jun 3, 2021

@jtgrohn yes, that would be helpful! Aiming to clear time for this, but would appreciate if you took a stab at it in the meantime 🙇🏾

@jcudit jcudit modified the milestones: v4.11.0, v4.12.0 Jun 7, 2021
@jcudit
Copy link
Contributor

jcudit commented Jun 18, 2021

@jtgrohn apologies for the delay on this one. I took another look at this and overall like the implementation. The tests however are not compatible with our test org as it does not support internal repositories.

Can the test be rewritten so that it:

  • reverts to the previous state of "creates repos with private visibility"
  • creates a separate, duplicate test for "creates repos with internal visibility"
  • skips the new internal visibility test

The hope here is we retain a manual way to test this locally while also allowing automated tests to continue passing. Open to other suggestions!

@jcudit jcudit modified the milestones: v4.12.0, v4.13.0 Jun 18, 2021
@pmjacinto
Copy link

Hey @jcudit, @jtgrohn thanks for this, we have a similar problem to #788.

When testing this change, I've realised that when creating a repository from a template it still creates the repository as private and later changes it to internal.

The REST API method for creating a repo from a template does not appear to support setting the visibility to internal as there's no visibility field in the docs.

The GraphQL API does support it though, through the cloneTemplateRepository mutation.

I have a version in my branch that builds on this PR and uses the GraphQL API when creating from a template. Would you welcome a PR on this?

@jtgrohn
Copy link
Contributor Author

jtgrohn commented Jun 21, 2021

@jtgrohn apologies for the delay on this one. I took another look at this and overall like the implementation. The tests however are not compatible with our test org as it does not support internal repositories.

Can the test be rewritten so that it:

  • reverts to the previous state of "creates repos with private visibility"
  • creates a separate, duplicate test for "creates repos with internal visibility"
  • skips the new internal visibility test

The hope here is we retain a manual way to test this locally while also allowing automated tests to continue passing. Open to other suggestions!

Done. Sorry I didn't get back to this PR earlier - I got sidelined with other priorities.

@jcudit jcudit merged commit 4f660b5 into integrations:master Jun 30, 2021
@jtgrohn jtgrohn deleted the test/internal-repository-fix-for-issue-788 branch July 5, 2021 12:24
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
…ing it later (integrations#794)

* create repository with internal visibility initially instead of changing it later

* revert to only a test for creates repos with private visibility

* add skippable test for creating internal repositories
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants