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

GitHub app support for TFE #655

Merged
merged 26 commits into from
Mar 3, 2023
Merged

GitHub app support for TFE #655

merged 26 commits into from
Mar 3, 2023

Conversation

roleesinhaHC
Copy link
Contributor

@roleesinhaHC roleesinhaHC commented Feb 22, 2023

Description

This PR adds the support of GitHub Apps as VCS provider for the Workflow, Policy Sets and Registry Module APIs.

The list of APIs that are added:

List GitHub App Installations( for the current user)
GET github-app/installations

Show an GitHub App Installation
GET /github-app/installation/:github-app-installation-id

In addition to that, all existing APIs to that support VCS connections are updated with GitHub App as a new provider.

The integration tests for thees changes will need the environment variable GITHUB_APP_INSTALLATION_ID to be setup. This environment variable is not included in the CI pipeline, so the integration tests will be skipped.

Testing plan

  1. TFE_ADDRESS=https://tfe.local TFE_TOKEN=xyz go test -run TestGHAInstallationList -v ./...
  2. GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx go test -run TestGHAInstallationRead -v ./...
  3. GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx GITHUB_POLICY_SET_IDENTIFIER=username/repository go test -run TestWorkspacesCreateTableDrivenWithGithubApp -v ./...
  4. GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx GITHUB_POLICY_SET_IDENTIFIER=username/repository go test -run TestWorkspacesUpdateTableDrivenWithGithubApp -v ./...
  5. GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx GITHUB_POLICY_SET_IDENTIFIER=username/repository GITHUB_REGISTRY_MODULE_IDENTIFIER=rusername/repository go test -run TestRegistryModulesCreateWithGithubApp -v ./...
  6. GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx GITHUB_POLICY_SET_IDENTIFIER=username/repository ENABLE_TFE=0 -run TestPolicySetsCreateWithGithubApp -v ./...

External links

Output from tests

Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.

$ TFE_ADDRESS="https://example" TFE_TOKEN="example" go test ./... -v -run TestFunctionsAffectedByChange

...

@roleesinhaHC roleesinhaHC changed the title Rolees/GitHub app support GitHub app support for TFE Feb 22, 2023
@roleesinhaHC roleesinhaHC marked this pull request as ready for review February 27, 2023 07:13
@roleesinhaHC roleesinhaHC requested a review from a team as a code owner February 27, 2023 07:13
@roleesinhaHC roleesinhaHC requested review from a team and mjyocca and removed request for a team February 27, 2023 07:14
Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

Can you also update the section "optional environment variables" in docs/TESTING.md with a short explanation about GITHUB_APP_INSTALLATION_ID (why it can't be automated, where to get one, maybe an example go test command that tests these features specifically if you do have one)

@roleesinhaHC
Copy link
Contributor Author

Screenshot 2023-02-28 at 12 46 47 PM

Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

Discussion items only

docs/TESTS.md Outdated
Comment on lines 55 to 72
```sh
$ TFE_ADDRESS=https://tfe.local TFE_TOKEN=xyz go test -run TestGHAInstallationList -v ./...`
```
```sh
$ GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx go test -run TestGHAInstallationRead -v ./...
```
```sh
$ GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx GITHUB_POLICY_SET_IDENTIFIER=username/repository go test -run TestWorkspacesCreateTableDrivenWithGithubApp -v ./...
```
```sh
$ GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx GITHUB_POLICY_SET_IDENTIFIER=username/repository go test -run TestWorkspacesUpdateTableDrivenWithGithubApp -v ./...
```
```sh
$ GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx GITHUB_POLICY_SET_IDENTIFIER=username/repository GITHUB_REGISTRY_MODULE_IDENTIFIER=username/repository go test -run TestRegistryModulesCreateWithGithubApp -v ./...
```
```sh
$ GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx GITHUB_POLICY_SET_IDENTIFIER=username/repository ENABLE_TFE=0 -run TestPolicySetsCreateWithGithubApp -v ./...
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is slightly overwhelming and I think the usecase will be to test these all at once. What do you think about
this suggestion?

Suggested change
```sh
$ TFE_ADDRESS=https://tfe.local TFE_TOKEN=xyz go test -run TestGHAInstallationList -v ./...`
```
```sh
$ GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx go test -run TestGHAInstallationRead -v ./...
```
```sh
$ GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx GITHUB_POLICY_SET_IDENTIFIER=username/repository go test -run TestWorkspacesCreateTableDrivenWithGithubApp -v ./...
```
```sh
$ GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx GITHUB_POLICY_SET_IDENTIFIER=username/repository go test -run TestWorkspacesUpdateTableDrivenWithGithubApp -v ./...
```
```sh
$ GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx GITHUB_POLICY_SET_IDENTIFIER=username/repository GITHUB_REGISTRY_MODULE_IDENTIFIER=username/repository go test -run TestRegistryModulesCreateWithGithubApp -v ./...
```
```sh
$ GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx GITHUB_POLICY_SET_IDENTIFIER=username/repository ENABLE_TFE=0 -run TestPolicySetsCreateWithGithubApp -v ./...
```
```sh
$ GITHUB_APP_INSTALLATION_ID=ghain-xxxx TFE_ADDRESS= https://tfe.local TFE_TOKEN=xxx GITHUB_POLICY_SET_IDENTIFIER=username/repository GITHUB_REGISTRY_MODULE_IDENTIFIER=username/repository go test -run "(GHA|GithubApp)" -v ./...
```

Here's what I get when I use that expression:

$ go test -list "(GHA|GithubApp)" ./...
TestGHAInstallationList
TestGHAInstallationRead
TestPolicySetsCreateWithGithubApp
TestRegistryModulesCreateWithGithubApp
TestWorkspacesCreateTableDrivenWithGithubApp
TestWorkspacesUpdateTableDrivenWithGithubApp
ok  	github.com/hashicorp/go-tfe	0.662s

Comment on lines 15 to 16
// GHAInstallations describes all the GitHub App Installation related methods that the
// Terraform Enterprise API supports.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should mention the peculiar user token requirements when using this endpoint.

docs/TESTS.md Outdated
4. `ENABLE_TFE` - Some tests are only applicable to Terraform Enterprise or Terraform Cloud. By setting `ENABLE_TFE=1` you will enable enterprise only tests and disable cloud only tests. In CI `ENABLE_TFE` is not set so if you are writing enterprise only features you should manually test with `ENABLE_TFE=1` against a Terraform Enterprise instance.
5. `ENABLE_BETA` - Some tests require access to beta features. By setting `ENABLE_BETA=1` you will enable tests that require access to beta features. IN CI `ENABLE_BETA` is not set so if you are writing beta only features you should manually test with `ENABLE_BETA=1` against a Terraform Enterprise instance with those features enabled.
6. `TFC_RUN_TASK_URL` - Run task integration tests require a URL to use when creating run tasks. To learn more about the Run Task API, [read here](https://developer.hashicorp.com/terraform/cloud-docs/api-docs/run-tasks/run-tasks)
7. `GITHUB_APP_INSTALLATION_ID` - Required for running any tests that use GitHub App as the VCS provider (workspace, policy sets, registry module). These tests are skipped in the automated CI pipeline because in order to use this variable, the user has to have a GitHub App Installation setup done using [TFC UI](https://developer.hashicorp.com/terraform/cloud-docs/vcs/github-app). And then the value can be fetched either from the UI or from the `GET /github-app/installations` API. The test commands are listed below.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
7. `GITHUB_APP_INSTALLATION_ID` - Required for running any tests that use GitHub App as the VCS provider (workspace, policy sets, registry module). These tests are skipped in the automated CI pipeline because in order to use this variable, the user has to have a GitHub App Installation setup done using [TFC UI](https://developer.hashicorp.com/terraform/cloud-docs/vcs/github-app). And then the value can be fetched either from the UI or from the `GET /github-app/installations` API. The test commands are listed below.
7. `GITHUB_APP_INSTALLATION_ID` - Required for running any tests that use GitHub App as the VCS provider (workspace, policy sets, registry module). These tests are skipped in the automated CI pipeline because in order to use this variable, the user has to have a GitHub App Installation setup done using [TFC UI](https://developer.hashicorp.com/terraform/enterprise/admin/application/github-app-integration). And then the value can be fetched either from the UI or from the `GET /github-app/installations` API. The test commands are listed below.

Comment on lines 34 to 37
CreateWithVCSConnection(ctx context.Context, options RegistryModuleCreateWithVCSConnectionOptions) (*RegistryModule, error)

// Create and publish a registry module with a VCS repo
CreateWithGithubApp(ctx context.Context, organization string, options RegistryModuleCreateWithVCSConnectionOptions) (*RegistryModule, error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this distinction isn't so important because the options are identical, with the only difference being that an org name is required for the URL. I wonder if it would be better to include organization in the RegistryModuleCreateWithVCSConnectionOptions, but only require it if GHAInstallationID is also present. Then you'd choose your URL based on which options are present, potentially preferring one over the other.

OR if you think there is a relevant distinction between VCS and GithubApp in the mind of users, then emphasize that by splitting the options into VCS options and GIthubApp options.

What do you think?

@roleesinhaHC roleesinhaHC requested a review from brandonc March 3, 2023 06:17
Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

$ GITHUB_APP_INSTALLATION_ID=ghain-519qPwh778M3ucmE GITHUB_POLICY_SET_IDENTIFIER="brandonc/test-policy-set" GITHUB_REGISTRY_MODULE_IDENTIFIER="brandonc/terraform-aws-simple-s3-cdn" go test -run "(GHA|GithubApp)" -v ./...
=== RUN   TestGHAInstallationList
=== RUN   TestGHAInstallationList/without_list_options
github-app/installations
--- PASS: TestGHAInstallationList (2.66s)
    --- PASS: TestGHAInstallationList/without_list_options (1.42s)
=== RUN   TestGHAInstallationRead
=== RUN   TestGHAInstallationRead/when_installation_id_exists
--- PASS: TestGHAInstallationRead (2.20s)
    --- PASS: TestGHAInstallationRead/when_installation_id_exists (1.15s)
=== RUN   TestPolicySetsCreateWithGithubApp
=== RUN   TestPolicySetsCreateWithGithubApp/with_vcs_policy_set
=== RUN   TestPolicySetsCreateWithGithubApp/with_vcs_policy_updated
--- PASS: TestPolicySetsCreateWithGithubApp (13.42s)
    --- PASS: TestPolicySetsCreateWithGithubApp/with_vcs_policy_set (4.41s)
    --- PASS: TestPolicySetsCreateWithGithubApp/with_vcs_policy_updated (3.34s)
=== RUN   TestRegistryModulesCreateWithGithubApp
=== RUN   TestRegistryModulesCreateWithGithubApp/with_valid_options
=== RUN   TestRegistryModulesCreateWithGithubApp/with_valid_options/permissions_are_properly_decoded
=== RUN   TestRegistryModulesCreateWithGithubApp/with_valid_options/relationships_are_properly_decoded
=== RUN   TestRegistryModulesCreateWithGithubApp/with_valid_options/timestamps_are_properly_decoded
=== RUN   TestRegistryModulesCreateWithGithubApp/with_invalid_options
=== RUN   TestRegistryModulesCreateWithGithubApp/with_invalid_options/without_an_github_app_installation_ID
=== RUN   TestRegistryModulesCreateWithGithubApp/with_invalid_options/without_an_org_name
=== RUN   TestRegistryModulesCreateWithGithubApp/without_options
--- PASS: TestRegistryModulesCreateWithGithubApp (5.91s)
    --- PASS: TestRegistryModulesCreateWithGithubApp/with_valid_options (1.10s)
        --- PASS: TestRegistryModulesCreateWithGithubApp/with_valid_options/permissions_are_properly_decoded (0.00s)
        --- PASS: TestRegistryModulesCreateWithGithubApp/with_valid_options/relationships_are_properly_decoded (0.00s)
        --- PASS: TestRegistryModulesCreateWithGithubApp/with_valid_options/timestamps_are_properly_decoded (0.00s)
    --- PASS: TestRegistryModulesCreateWithGithubApp/with_invalid_options (0.00s)
        --- PASS: TestRegistryModulesCreateWithGithubApp/with_invalid_options/without_an_github_app_installation_ID (0.00s)
        --- PASS: TestRegistryModulesCreateWithGithubApp/with_invalid_options/without_an_org_name (0.00s)
    --- PASS: TestRegistryModulesCreateWithGithubApp/without_options (0.00s)
=== RUN   TestWorkspacesCreateTableDrivenWithGithubApp
=== RUN   TestWorkspacesCreateTableDrivenWithGithubApp/when_options_include_tags-regex
--- PASS: TestWorkspacesCreateTableDrivenWithGithubApp (14.57s)
    --- PASS: TestWorkspacesCreateTableDrivenWithGithubApp/when_options_include_tags-regex (6.58s)
=== RUN   TestWorkspacesUpdateTableDrivenWithGithubApp
=== RUN   TestWorkspacesUpdateTableDrivenWithGithubApp/when_options_include_VCSRepo_tags-regex
--- PASS: TestWorkspacesUpdateTableDrivenWithGithubApp (23.79s)
    --- PASS: TestWorkspacesUpdateTableDrivenWithGithubApp/when_options_include_VCSRepo_tags-regex (12.58s)
PASS
ok  	github.com/hashicorp/go-tfe	62.789s
?   	github.com/hashicorp/go-tfe/examples/configuration_versions	[no test files]
?   	github.com/hashicorp/go-tfe/examples/organizations	[no test files]
?   	github.com/hashicorp/go-tfe/examples/registry_modules	[no test files]
?   	github.com/hashicorp/go-tfe/examples/workspaces	[no test files]
?   	github.com/hashicorp/go-tfe/mocks	[no test files]

@brandonc
Copy link
Collaborator

brandonc commented Mar 3, 2023

Would you mind squashing when you merge?

@roleesinhaHC
Copy link
Contributor Author

Would you mind squashing when you merge?

Yes, absolutely. Thanks @brandonc !

@roleesinhaHC roleesinhaHC merged commit f4b78f0 into main Mar 3, 2023
@roleesinhaHC roleesinhaHC deleted the rolees/github_app_support branch March 3, 2023 17:02
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

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