-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(codebuild): add startBatchBuild
option
#11743
Conversation
@skinny85 do you know why I guess somewhere else needs updating? Edit: nevermind, Just spotted the problem :| |
startBatchBuild
optionstartBatchBuild
option
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.
Awesome contribution @tjenkinson ! Just a few questions/comments before I "ShipIt".
@@ -482,6 +482,13 @@ interface ThirdPartyGitSourceProps extends GitSourceProps { | |||
*/ | |||
readonly webhook?: boolean; | |||
|
|||
/** | |||
* Start a batch build when the webhook is triggered instead of a standard one. |
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.
The "when the webhook is triggered" is an interesting statement. So this setting works only for builds started through the webhook? Not through, for instance, CloudWatch Events?
If that is indeed the case, I'd like to change this a little bit:
- Let's call this property
webhookTriggersBatchBuild?: boolean
instead, to make it absolutely clear what it does. - If
webhookTriggersBatchBuild
is passed astrue
, butwebhook
isfalse
(which gets a little complicated becausewebhook
is consideredtrue
if it'sundefined
, butwebhookFilters
have been provided), that sounds like an error to me, and we should validate that scenario (and check that logic with a separate unit test, of course 🙂).
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.
Hmm good question. I assume it applies for anything that triggers. Asked here: aws-cloudformation/cloudformation-coverage-roadmap#621 (comment)
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.
@tjenkinson can you perhaps do an experiment with that setting and CloudWatch Events, and report back what you find? I don't want to hold your PR hostage to this question 🙂.
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.
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.
Looking at the docs here I don't think it's possible to trigger a batch build from cloudwatch, because it mentions that any options you provide are for the StartBuild
call, not StartBuildBatch
For more information about the parameters you can pass, see StartBuild. You cannot pass the projectName parameter in this field. Instead, you specify the project using the ARN in Project ARN.
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.
@tjenkinson OK. Let's assume that it is the case then, and let's implement the 2 points I made here #11743 (comment) .
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.
@skinny85 currently if you pass webhooks: false
and also provide filter groups, then the end result is no error, and the SourceConfig
contains webhook
false
and the filter groups.
So I followed the same logic and decided to default webhooks
to true
if it was omitted and there were filter groups or webhookTriggersBatchBuild
was true
.
Also if you explicitly set webhooks
to false
we will leave it as false
but still set the buildType
in the SourceConfig
just like we do with the filters.
WDYT?
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 feel like passing webhooks: false
, but webhookFilters
is probably a mistake...
In fact, I just tried it, and I got the following error from CodeBuild's API:
Failed to call CreateWebhook, reason: Cannot specify filter groups when webhook is disabled.
(Service: null; Status Code: 400; Error Code: null; Request ID: null; Proxy: null)
This makes me believe that adding this validation for webhookTriggersBatchBuild
is the correct thing to do (I won't bother you with adding it for webhookFilters
, as it's not really the topic of this PR).
Pull request has been modified.
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
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.
Putting in "Request changes" to remove it from y ToDo-list. @tjenkinson when you've updated the PR, please make sure to re-request my review!
Pull request has been modified.
Oh build failed. Will look into that. It worked locally |
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.
Fantastic contribution @tjenkinson ! Fix the behavior for webhookTriggersBatchBuild
being true
, but webhook
being given as false
(it should throw an exception), and I'll gladly merge this one in 🙂.
Thanks,
Adam
Pull request has been modified.
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.
Thanks for the contribution @tjenkinson !
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
This adds a `startBatchBuild` option to the code build source, to trigger a batch build. The cloudformation property isn't in the official docs yet but is mentioned [here](aws-cloudformation/cloudformation-coverage-roadmap#621 (comment)). Closes aws#11663
This adds a
startBatchBuild
option to the code build source, to trigger a batch build. The cloudformation property isn't in the official docs yet but is mentioned here.Closes #11663