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

feat(iotevents): add IoT Events input L2 Construct #17847

Merged
merged 7 commits into from
Dec 11, 2021

Conversation

yamatatsu
Copy link
Contributor

This is proposed by #17711.

This PR was created for implemeting Input L2 Construct. Implementing it is needed before DetectorModel. 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

@gitpod-io
Copy link

gitpod-io bot commented Dec 5, 2021

@github-actions github-actions bot added the @aws-cdk/aws-iotevents Related to AWS IoT Events label Dec 5, 2021
@pahud
Copy link
Contributor

pahud commented Dec 7, 2021

Hi @minche-tsai are you able to take a first look?

Copy link
Contributor

@skinny85 skinny85 left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

😃

Copy link
Contributor Author

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 {
Copy link
Contributor

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 })),
Copy link
Contributor

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 []?

Copy link
Contributor Author

@yamatatsu yamatatsu Dec 8, 2021

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!

@mergify mergify bot dismissed skinny85’s stale review December 8, 2021 10:18

Pull request has been modified.

@yamatatsu yamatatsu requested a review from skinny85 December 8, 2021 11:58
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Fantastic work @yamatatsu!

packages/@aws-cdk/aws-iotevents/lib/input.ts Outdated Show resolved Hide resolved
skinny85
skinny85 previously approved these changes Dec 10, 2021
@mergify mergify bot dismissed skinny85’s stale review December 10, 2021 22:06

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Dec 10, 2021

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: fb25883
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 9f03dc4 into aws:master Dec 11, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 11, 2021

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).

@yamatatsu yamatatsu deleted the iotevents-input branch December 14, 2021 13:22
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iotevents Related to AWS IoT Events
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants