-
Notifications
You must be signed in to change notification settings - Fork 754
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
Conversation
Update fork to match upstream
Looking great so far! A couple of suggestions ahead of merging this:
❤️ the direction here. Feels like we're closer to shipping #509 and #514 with this feature in place. Thanks for driving this! |
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! |
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 |
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. |
There was a problem hiding this 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 🙇🏾 .
Create: resourceGithubAppInstallationRepositoryCreate, | ||
Read: resourceGithubAppInstallationRepositoryRead, | ||
Delete: resourceGithubAppInstallationRepositoryDelete, |
There was a problem hiding this comment.
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?
# 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}" | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
* 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
Example Usage:
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!