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

Added WebhookURL to VCSRepo struct #413

Merged
merged 5 commits into from Jun 9, 2022

Conversation

kgns
Copy link
Contributor

@kgns kgns commented May 24, 2022

Description

Terraform creates a webhook URL for every workspace that is attached to a VCS Provider, but for some reason, this generated URL is not exposed to the users of the go-tfe client. This PR tries to fix that. If this PR gets merged, will need to wait for a new release and then send another PR to https://github.com/hashicorp/terraform-provider-tfe which would also expose the same information to TFE provider users.

This will be useful for people (at least for me) who wants to add custom logic between GitHub (or any other VCS) and Terraform Cloud/Enterprise.

I had to skip the test for the Policy Sets as I currently do not have a paid membership on TF Cloud.

Output from tests

=== RUN   TestRegistryModulesCreateWithVCSConnection
=== RUN   TestRegistryModulesCreateWithVCSConnection/with_valid_options
=== RUN   TestRegistryModulesCreateWithVCSConnection/with_valid_options/permissions_are_properly_decoded
=== RUN   TestRegistryModulesCreateWithVCSConnection/with_valid_options/relationships_are_properly_decoded
=== RUN   TestRegistryModulesCreateWithVCSConnection/with_valid_options/timestamps_are_properly_decoded
=== RUN   TestRegistryModulesCreateWithVCSConnection/with_invalid_options
=== RUN   TestRegistryModulesCreateWithVCSConnection/with_invalid_options/without_an_identifier
=== RUN   TestRegistryModulesCreateWithVCSConnection/with_invalid_options/without_an_oauth_token_ID
=== RUN   TestRegistryModulesCreateWithVCSConnection/with_invalid_options/without_a_display_identifier
=== RUN   TestRegistryModulesCreateWithVCSConnection/without_options
--- PASS: TestRegistryModulesCreateWithVCSConnection (3.53s)
    --- PASS: TestRegistryModulesCreateWithVCSConnection/with_valid_options (0.80s)
        --- PASS: TestRegistryModulesCreateWithVCSConnection/with_valid_options/permissions_are_properly_decoded (0.00s)
        --- PASS: TestRegistryModulesCreateWithVCSConnection/with_valid_options/relationships_are_properly_decoded (0.00s)
        --- PASS: TestRegistryModulesCreateWithVCSConnection/with_valid_options/timestamps_are_properly_decoded (0.00s)
    --- PASS: TestRegistryModulesCreateWithVCSConnection/with_invalid_options (0.00s)
        --- PASS: TestRegistryModulesCreateWithVCSConnection/with_invalid_options/without_an_identifier (0.00s)
        --- PASS: TestRegistryModulesCreateWithVCSConnection/with_invalid_options/without_an_oauth_token_ID (0.00s)
        --- PASS: TestRegistryModulesCreateWithVCSConnection/with_invalid_options/without_a_display_identifier (0.00s)
    --- PASS: TestRegistryModulesCreateWithVCSConnection/without_options (0.00s)
PASS
ok      github.com/hashicorp/go-tfe     (cached)
?       github.com/hashicorp/go-tfe/examples/organizations      [no test files]
?       github.com/hashicorp/go-tfe/examples/workspaces [no test files]
?       github.com/hashicorp/go-tfe/mocks       [no test files]


=== RUN   TestRegistryModule_Unmarshal
--- PASS: TestRegistryModule_Unmarshal (0.00s)
PASS
ok      github.com/hashicorp/go-tfe     (cached)
?       github.com/hashicorp/go-tfe/examples/organizations      [no test files]
?       github.com/hashicorp/go-tfe/examples/workspaces [no test files]
?       github.com/hashicorp/go-tfe/mocks       [no test files]


=== RUN   TestWorkspace_Unmarshal
--- PASS: TestWorkspace_Unmarshal (0.00s)
PASS
ok      github.com/hashicorp/go-tfe     (cached)
?       github.com/hashicorp/go-tfe/examples/organizations      [no test files]
?       github.com/hashicorp/go-tfe/examples/workspaces [no test files]
?       github.com/hashicorp/go-tfe/mocks       [no test files]

Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

Code overall looks great 👍 🔥 , some minor things to change down below. ⬇️

assert.Equal(t, ps.VCSRepo.OAuthTokenID, oc.ID)
assert.Equal(t, ps.VCSRepo.RepositoryHTTPURL, fmt.Sprintf("https://github.com/%s", githubIdentifier))
assert.Equal(t, ps.VCSRepo.ServiceProvider, string(ServiceProviderGithub))
assert.Regexp(t, "^https://app\\.terraform\\.io/webhooks/vcs/[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}$", ps.VCSRepo.WebhookURL)
Copy link
Contributor

Choose a reason for hiding this comment

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

The only thing I might change here is the host, given that someone can test against a Terraform Enterprise installation where the host might not necessarily match with app...terraform...io. The best choice here would be to stick the value of TFE_ADDRESS into the regexp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I see the problem. Is the rest of the URL for the webhook standard or can the endpoint and/or the UUID part also change on TFE?

Copy link
Contributor Author

@kgns kgns May 25, 2022

Choose a reason for hiding this comment

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

I couldn't find a way to easily access the Config object used to create the test client, so I called DefaultConfig() myself for its Address field. DefaultConfig() function handles all cases with TFE_ADDRESS and TFE_HOSTNAME env variables (or lack thereof) so I thought it would be best to use it instead of just TFE_ADDRESS. If there is a better/cleaner way, feel free to point me in the right direction

assert.Equal(t, ps.VCSRepo.OAuthTokenID, oc.ID)
assert.Equal(t, ps.VCSRepo.RepositoryHTTPURL, fmt.Sprintf("https://github.com/%s", githubIdentifier))
assert.Equal(t, ps.VCSRepo.ServiceProvider, string(ServiceProviderGithub))
assert.Regexp(t, "^https://app\\.terraform\\.io/webhooks/vcs/[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}$", ps.VCSRepo.WebhookURL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, here as well!

@kgns kgns requested a review from sebasslash May 25, 2022 23:44
@kgns
Copy link
Contributor Author

kgns commented May 26, 2022

closes #138

Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

👍 Awesome

@kgns
Copy link
Contributor Author

kgns commented Jun 8, 2022

Is there something wrong with the circleci tests? #422 also has some fails but the only change is an addition to the change log

@annawinkler
Copy link
Contributor

Is there something wrong with the circleci tests? #422 also has some fails but the only change is an addition to the change log

I'll start the tests again. We have a few flaky tests.

@sebasslash sebasslash merged commit 7f5d83c into hashicorp:main Jun 9, 2022
@github-actions
Copy link

github-actions bot commented Jun 9, 2022

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.

None yet

3 participants