-
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(iotevents): add IoT Events input L2 Construct #17847
Conversation
Hi @minche-tsai are you able to take a first look? |
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.
Looks great as always @yamatatsu! A few minor comments.
/** | ||
* The name of the input | ||
* | ||
* @default xxx |
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.
🙇🏻
/** | ||
* Defines an AWS IoT Events input in this stack. | ||
*/ | ||
export class Input extends 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.
Looks like we're missing the IInput
interface for Input
.
const resource = new CfnInput(this, 'Resource', { | ||
inputName: this.physicalName, | ||
inputDefinition: { | ||
attributes: props.attributeJsonPaths.map(path => ({ jsonPath: path })), |
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.
Can attributeJsonPaths
be empty? If not, can we validate that it's not? If it can be empty, can we make it optional, and default to []
?
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.
As you said, attributeJsonPaths
can not be empty. So I've been thinking that it is better to validate it too!
On the other hand, I thought I had to make my initial L2 PR smaller so as not to repeat my previous mistakes😅 .
I never mind which ways, implement in this PR or after! 😉
I've fixed it!
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.
Fantastic work @yamatatsu!
Pull request has been modified.
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 is proposed by aws#17711. This PR was created for implemeting `Input` L2 Construct. Implementing it is needed before `DetectorModel`. The reason is described in here: aws#17711 (comment) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This is proposed by #17711.
This PR was created for implemeting
Input
L2 Construct. Implementing it is needed beforeDetectorModel
. The reason is described in here: #17711 (comment)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license