-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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 aws_codebuild_webhook resource for creating GitHub webhooks for CodeBuild projects #4473
Conversation
…aform-provider-aws into jstump-add-codepipeline-webhook
Not sure this will help much without tracking the For example, this is the syntax I'd expect:
Unfortunately, it doesn't look like the So if the secret can be stored in the state, that'd be ideal. Awesome to see progress on this - it'd be a huge win for our devops stack. |
I'll be taking a look at this tomorrow. 👍 |
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.
Pull request review below for those curious -- I will be handling the feedback for this one and getting this merged shortly. 😄
// Additionally, the GitHub user that the Terraform AWS user logs in as must have | ||
// access to the GitHub repository. This allows others to run tests for the webhook | ||
// without having to have access to the Packer GitHub repository. | ||
sourceURL, ok := os.LookupEnv("CODEBUILD_GITHUB_SOURCE_URL") |
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 is a great idea -- I'll make sure this gets ported over in some fashion when I rebase this PR.
Update: resourceAwsCodeBuildWebhookUpdate, | ||
|
||
Schema: map[string]*schema.Schema{ | ||
"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.
This might seem pedantic for now, but on merge will rename this attribute to project_name
to better align with the API and help operators understand the association with projects.
} | ||
|
||
d.SetId(d.Get("name").(string)) | ||
d.Set("branch_filter", d.Get("branch_filter").(string)) |
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.
d.Set()
should only be called in the read function 😄 -- will fix this on merge.
}) | ||
|
||
if err != nil { | ||
return err |
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.
We should check for "resource not found" type errors and d.SetId("")
similar to how the project resource does this. Will fix on merge. 👍
} | ||
|
||
if len(resp.Projects) == 0 { | ||
d.SetId("") |
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.
When we're removing a resource from the state, we should log an appropriate warning message in the logs, e.g.
log.Printf("[WARN] CodeBuild Project %q not found, removing from state", d.Id())
Config: testAccCodeBuildWebhookConfig_basic(acctest.RandString(5)), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckAwsCodeBuildWebhookExists("aws_codebuild_webhook.test"), | ||
resource.TestCheckResourceAttrSet("aws_codebuild_webhook.test", "url"), |
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.
We should probably be a little more cautious and at least partially validate the value as resource.TestCheckResourceAttrSet()
will pass with empty strings and I believe even d.Set("XXX", nil)
e.g.
resource.TestMatchResourceAttr("aws_codebuild_webhook.test", "url", regexp.MustCompile(`^https://`))
}) | ||
|
||
if err != nil { | ||
return err |
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.
A resource not found exception here would be a good thing and should return nil
-- will fix on merge.
if !ok { | ||
return fmt.Errorf("Not found: %s", 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.
Similar to how the destroy acceptance test function above is checking against the API, we should do the same thing here to ensure the "physical" webhook actually exists.
|
||
Provides a CodeBuild Webhook resource. | ||
|
||
~> **Note:** The AWS account that Terraform uses to create this resource *must* have authorized CodeBuild to access GitHub's OAuth API. This is a manual step that must be done *before* creating webhooks with this resource. If OAuth is not configured, AWS will return an error similar to `ResourceNotFoundException: Could not find access token for server type github`. |
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 manual step must also be done in every applicable region where the resource will be used. We should probably also point to the AWS documentation surrounding project webhooks here. Will update on merge.
"github.com/hashicorp/terraform/helper/schema" | ||
) | ||
|
||
func resourceAwsCodeBuildWebhook() *schema.Resource { |
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.
We can easily add import support for this resource as the ID is already handled properly in the read function. Will add the passthrough importer to this resource and document how to import on merge.
AWS is using "your" GitHub OAuth credentials to manage the actual GitHub webhook (physically creating, updating, and deleting on the GitHub side with this resource). Edit: Ah, but upon further reading of the documentation -- this looks like its required for GitHub Enterprise. I'll make sure the additional attributes are exported. 👍 Sorry for jumping the gun. |
The new |
Hooray! This is a big improvement for our stacks. Thanks @bflad, @atsushi-ishibashi @joestump! |
@bflad thanks for the notes and merge! 🎉 |
This has been released in version 1.21.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
I think the documentation needs to be updated. It still list only |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Changes proposed in this pull request:
Introduces a resource called
aws_codebuild_webhook
that creates GitHub webhook for a CodeBuild project that has its source stored on GitHub.This works builds on #2814 and adds branch filtering.
Output from acceptance testing:
The reason because this fails is because:
source
.I've tried to test this, but passing
AWS_ACCESS_KEY_ID
andAWS_SECRET_ACCESS_KEY
to tests appears to be ignored. Creating a webhook with the credentials I created works via the CLI, but not via the API call this resource makes.How does one use their own AWS credentials when running
make test
?NOTE: Once this passes, the upstream Terraform test environment will need to address this in order to run these tests. Alternatively, I could implement something like the Heroku provider has for skipping tests.