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 github_app_installation_repository resource #690

Merged
merged 9 commits into from
Mar 23, 2021

Conversation

k24dizzle
Copy link
Contributor

Example Usage:

resource "github_app_installation_repository" "example" {
    repository      = "testrepo"
    installation_id = "1234567"
}

Gave #687 a shot. However, while this implementation works in my local testing (I was able to install an existing app to existing repos), this PR is incomplete.

I was going to write acceptance tests for this PR, but realized that there is no way to spin up an organization that already has an app installed. We would need to write a corresponding resource that installs github apps in an organization, but I don't believe that there is an easy way to do that through the Github API.

Any ideas on potential next steps/feedback would be greatly appreciated!

@jcudit
Copy link
Contributor

jcudit commented Feb 5, 2021

Looking great so far! A couple of suggestions ahead of merging this:

  • Since this is currently difficult to acceptance test, we could benefit from a new example in the examples/ directory
  • We'll need to document this similar to other resources (see website/docs/r and website/github.erb)

❤️ the direction here. Feels like we're closer to shipping #509 and #514 with this feature in place. Thanks for driving this!

@k24dizzle
Copy link
Contributor Author

Thanks for the comments @jcudit, went ahead and implemented your feedback. Let me know if I can do anything else/if you have more feedback!

@k24dizzle
Copy link
Contributor Author

Hi @jcudit I was wondering if there were any updates to this PR, feel free to let me know if there's any more work to be done on this

@jcudit
Copy link
Contributor

jcudit commented Mar 8, 2021

Thanks for the ping 🙇🏾 . A few of us are going to pair on reviews this week. Aiming to get this one tested out during the session and will likely ship with the next release.

@jcudit jcudit added this to the v4.6.0 milestone Mar 8, 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.

Excited to see this one land. Thanks for getting it this far and being patient on the feedback 🙇🏾 .

Comment on lines +14 to +16
Create: resourceGithubAppInstallationRepositoryCreate,
Read: resourceGithubAppInstallationRepositoryRead,
Delete: resourceGithubAppInstallationRepositoryDelete,
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 would we benefit from an Update function here as well? Or is creating a new App installation usually non-disruptive?

Comment on lines +23 to +32
# Create a repository.
resource "github_repository" "some_repo" {
name = "some-repo"
}

resource "github_app_installation_repository" "some_app_repo" {
# The installation id of the app (in the organization).
installation_id = "1234567"
repository = "${github_repository.some_repo.name}"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get a test that exercises this configuration? Open to updates in HCL syntax as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤦🏾 I now see an earlier comment where I requested an example over an acceptance test. I'll give testing a shot and plan to move forward with this with just the example if the test does not pan out.

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 tested well locally and I've got eaaaf65 queued up to commit after this merges as-is. Thanks again for this contribution and your patience up to this point @k24dizzle.

@jcudit jcudit merged commit ad11c76 into integrations:master Mar 23, 2021
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
* Add github_app_installation_repository resource

* Address lint

* Add website documentation

* More consistent formatting + helpful link

* fix link

* Add example

* remove empty line

* Better readme
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.

2 participants