Add resource github_app_installation_repositories #1376

david-bain · 2022-11-21T05:58:01Z

This resource allows multiple repositories to be passed in; which greatly improves the performance of the resource compared to the single repository version when needing to control state of multiple app installations with multiple repos, required in larger organisations.

The optimisation occurs as only a single call to get the list of repos is required per installation per read, regardless of the number of repositories being added.

Add resource_github_app_installation_repositories
Add tests
update docs

Resolves N/A

Behavior

Before the change?

resource_github_app_installation_repostiory was a 1:1 installation:repo; however in reality, many repositories can be added to an installations allow-list. This meant that an app installation allowlist with say 5 repositories required 5 resources to be created/managed. Inside that resource, there was some generic calls that related more to the installation so if an app installation had multiple repositories allowed, those exact same API calls are repeated multiple times, e.g. client.Apps.ListUserRepos().
This made managing app allow-lists this way VERY slow

After the change?

Creates a new resource that allows multiple repositories to be defined against an installation in a 1:Many fashion. Following from the above, this means that the installation API calls are only called once per resource and as there are much less resources now due to the grouping of repositories, the terraform runs MUCH faster.
In our organisation, it is 30x faster this way to plan

Other information

This resource has been running within our org on a fork for approximately 3 months and has been very successful.

Additional info

Pull request checklist

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

Yes (Please add the Type: Breaking change label)
No

If Yes, what's the impact:

N/A

Pull request type

Please add the corresponding label for change this PR introduces:

Bugfix: Type: Bug
Feature/model/API additions: Type: Feature
Updates to docs or samples: Type: Documentation
Dependencies/code cleanup: Type: Maintenance

This resource allows multiple repositories to be passed in; which greatly improves the performance of the resource compared to the single repository version when needing to control state of multiple app installations with multiple repos, required in larger organisations. The optimisation occurs as only a single call to get the list of repos is required per installation per read, regardless of the number of respositories being added. - Add resource_github_app_installation_repositories - Add tests

kfcampbell

Hi @david-bain, this is super cool and thanks for opening a PR to share it! I have a couple questions:

Do you mind adding a link to the new documentation in the github.erb file so the new doc is accessible?
What am I missing about running the integration tests? I've exported a valid APP_INSTALLATION_ID retrieved from my organization's app installation settings, but my tests fail with the following error:

    testing.go:705: Step 0 error: errors during apply:
        
        Error: GET https://api.github.com/user/installations/131977/repositories?per_page=100: 404 Not Found []

Do you think this could be permission-related?

Would you mind putting a note at the top of the docs for the new resource and resource_github_app_installation_repository that distinguishes between the purpose of each resource and describes when you might want to use one over the other?

Come to think of it, it may even be a good idea to deprecate resource_github_app_installation_repository soon in favor of the new resource.

kfcampbell · 2022-11-28T22:43:28Z

website/docs/r/app_installation_repositories.html.markdown

+* `installation_id`       - (Required) The GitHub app installation id.
+* `selected_repositories` - (Required) A list of repository names to install the app on.
+
+~> **Note**: Due to GitHub API limitations, deleting this resource will leave one repository with the app installed. Manually uninstall the app after deleting the resource.


How is the remaining repository determined? Is it deterministic?

See comment below, I'll update the docs to reflect

@kfcampbell
Re running the tests: I just pulled my branch and made sure to set GITHUB_TOKEN, GITHUB_ORGANISATION, and APP_INSTALLATION_ID and it ran fine.
I think I see your error. That ID in the path looks too small for an installation ID... I suspect you exported the App ID by accident. The installation ID is retrieved from the end of the web address when you navigate to a specific installation of an app.

Oh my gosh you're totally right! Thanks for the help troubleshooting the testing.

david-bain · 2022-12-01T00:58:39Z

Let me check into the above, I wrote this a while back on a fork so I need to refresh my memory regarding the remaining repo and tests.

david-bain · 2022-12-01T01:04:10Z

github/resource_github_app_installation_repositories.go

+	// Remove repositories that existed on GitHub but not selectedRepositories
+	// There is a github limitation that means we can't remove the last repository from an installation.
+	// Therefore, we skip the first and delete the rest. The app will then need to be uninstalled via the GUI
+	// as there is no current API endpoint for [un]installation. Ensure there is at least one repository remaining.


As per the comment here, I skip the first... which is based on the order of repositories that are returned by the the list accessible repos API call.
I guess I could sort it alphabetically so that it is always the first alphabetical by name that remains but not sure how useful that is.

This seems like a reasonable call. It's definitely an instance where GitHub's behavior doesn't fit a tidy CRUD model.

david-bain · 2022-12-01T22:39:18Z

Docs updated.
I believe I have addressed all your points. Please let me know if there is anything else.

kfcampbell

Thanks for making changes on this! I'll get this merged now and released in the near future.

* Add resource github_app_installation_repositories This resource allows multiple repositories to be passed in; which greatly improves the performance of the resource compared to the single repository version when needing to control state of multiple app installations with multiple repos, required in larger organisations. The optimisation occurs as only a single call to get the list of repos is required per installation per read, regardless of the number of respositories being added. - Add resource_github_app_installation_repositories - Add tests * Update docs and link Co-authored-by: Keegan Campbell <me@kfcampbell.com>

* feat: add new schema for check block resource under required_status_checks * feat: iterate provided `check` blocks and build array of RequiredStatusCheck * feat: set default app_id to -1 * feat: implement checks flattening for required status checks * Add resource github_app_installation_repositories (#1376) * Add resource github_app_installation_repositories This resource allows multiple repositories to be passed in; which greatly improves the performance of the resource compared to the single repository version when needing to control state of multiple app installations with multiple repos, required in larger organisations. The optimisation occurs as only a single call to get the list of repos is required per installation per read, regardless of the number of respositories being added. - Add resource_github_app_installation_repositories - Add tests * Update docs and link Co-authored-by: Keegan Campbell <me@kfcampbell.com> * feat: adds new branch protection options for last reviewer and locking branch (#1407) Co-authored-by: Keegan Campbell <me@kfcampbell.com> * feat(github_release): adding github_release resource and tests (#1122) * feat(github_release): adding github_release resource and tests * feat(docs) adding github_release page to website docs * chore: update changelog with this pr's new resource * fix: adding node_id and release_id to resource attributes * Update CHANGELOG.md * Fix broken merge/build Co-authored-by: Keegan Campbell <me@kfcampbell.com> * 🚧 Workflows have changed Workflow changes have been made in the Octokit org repo. This PR is propagating those changes. * Issue template tweak (#1422) * Don't link to a real PR * Wording tweak * feat: allow branch protection check app_id to be null * chore: change branch protection flatten function to use GetAppID sdk method * feat: change branch protection v3 utils to flatten and expand contexts into checks * feat: change checks from it's own resource to a list of strings * chore: resolve incorrect merge of main * chore: update deprecation notice on contexts array * chore(docs): Update branch_protection_v3 docs to mention the new `checks` functionality * fix: Initialise literal empty slice of RequiredStatusCheck to mitigate errors when passing nil to the sdk * chore(lint): resolve gosimple S1082 violation (errors.New => fmt.Errorf) * chore: remove unused code comment Co-authored-by: David Bain <97858950+david-bain@users.noreply.github.com> Co-authored-by: Keegan Campbell <me@kfcampbell.com> Co-authored-by: Sean Smith <sean@wwsean08.com> Co-authored-by: Trent Millar <trent.millar@gmail.com> Co-authored-by: Nick Floyd <139819+nickfloyd@users.noreply.github.com>

* Add resource github_app_installation_repositories This resource allows multiple repositories to be passed in; which greatly improves the performance of the resource compared to the single repository version when needing to control state of multiple app installations with multiple repos, required in larger organisations. The optimisation occurs as only a single call to get the list of repos is required per installation per read, regardless of the number of respositories being added. - Add resource_github_app_installation_repositories - Add tests * Update docs and link Co-authored-by: Keegan Campbell <me@kfcampbell.com>

* feat: add new schema for check block resource under required_status_checks * feat: iterate provided `check` blocks and build array of RequiredStatusCheck * feat: set default app_id to -1 * feat: implement checks flattening for required status checks * Add resource github_app_installation_repositories (integrations#1376) * Add resource github_app_installation_repositories This resource allows multiple repositories to be passed in; which greatly improves the performance of the resource compared to the single repository version when needing to control state of multiple app installations with multiple repos, required in larger organisations. The optimisation occurs as only a single call to get the list of repos is required per installation per read, regardless of the number of respositories being added. - Add resource_github_app_installation_repositories - Add tests * Update docs and link Co-authored-by: Keegan Campbell <me@kfcampbell.com> * feat: adds new branch protection options for last reviewer and locking branch (integrations#1407) Co-authored-by: Keegan Campbell <me@kfcampbell.com> * feat(github_release): adding github_release resource and tests (integrations#1122) * feat(github_release): adding github_release resource and tests * feat(docs) adding github_release page to website docs * chore: update changelog with this pr's new resource * fix: adding node_id and release_id to resource attributes * Update CHANGELOG.md * Fix broken merge/build Co-authored-by: Keegan Campbell <me@kfcampbell.com> * 🚧 Workflows have changed Workflow changes have been made in the Octokit org repo. This PR is propagating those changes. * Issue template tweak (integrations#1422) * Don't link to a real PR * Wording tweak * feat: allow branch protection check app_id to be null * chore: change branch protection flatten function to use GetAppID sdk method * feat: change branch protection v3 utils to flatten and expand contexts into checks * feat: change checks from it's own resource to a list of strings * chore: resolve incorrect merge of main * chore: update deprecation notice on contexts array * chore(docs): Update branch_protection_v3 docs to mention the new `checks` functionality * fix: Initialise literal empty slice of RequiredStatusCheck to mitigate errors when passing nil to the sdk * chore(lint): resolve gosimple S1082 violation (errors.New => fmt.Errorf) * chore: remove unused code comment Co-authored-by: David Bain <97858950+david-bain@users.noreply.github.com> Co-authored-by: Keegan Campbell <me@kfcampbell.com> Co-authored-by: Sean Smith <sean@wwsean08.com> Co-authored-by: Trent Millar <trent.millar@gmail.com> Co-authored-by: Nick Floyd <139819+nickfloyd@users.noreply.github.com>

kfcampbell added the Type: Feature New feature or request label Nov 28, 2022

Merge branch 'main' into add-app-repositories

5c7d7f1

kfcampbell reviewed Nov 28, 2022

View reviewed changes

david-bain commented Dec 1, 2022

View reviewed changes

Update docs and link

52cab92

kfcampbell added 2 commits December 1, 2022 16:28

Merge branch 'main' into add-app-repositories

8e4208a

Merge branch 'main' into add-app-repositories

3e13293

kfcampbell approved these changes Dec 5, 2022

View reviewed changes

kfcampbell merged commit 3db3e2f into integrations:main Dec 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resource github_app_installation_repositories #1376

Add resource github_app_installation_repositories #1376

david-bain commented Nov 21, 2022

kfcampbell left a comment

kfcampbell Nov 28, 2022

david-bain Dec 1, 2022

david-bain Dec 1, 2022 •

edited

Loading

kfcampbell Dec 5, 2022

david-bain commented Dec 1, 2022

david-bain Dec 1, 2022 •

edited

Loading

kfcampbell Dec 5, 2022

david-bain commented Dec 1, 2022

kfcampbell left a comment

Add resource github_app_installation_repositories #1376

Add resource github_app_installation_repositories #1376

Conversation

david-bain commented Nov 21, 2022

Behavior

Before the change?

After the change?

Other information

Additional info

Pull request checklist

Does this introduce a breaking change?

Pull request type

kfcampbell left a comment

Choose a reason for hiding this comment

kfcampbell Nov 28, 2022

Choose a reason for hiding this comment

david-bain Dec 1, 2022

Choose a reason for hiding this comment

david-bain Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

kfcampbell Dec 5, 2022

Choose a reason for hiding this comment

david-bain commented Dec 1, 2022

david-bain Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

kfcampbell Dec 5, 2022

Choose a reason for hiding this comment

david-bain commented Dec 1, 2022

kfcampbell left a comment

Choose a reason for hiding this comment

david-bain Dec 1, 2022 •

edited

Loading

david-bain Dec 1, 2022 •

edited

Loading