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.

Sign up for GitHub

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

Jump to bottom

feat(iotevents): add grant method to Input class #18617

Merged
merged 18 commits into from
Feb 3, 2022
Merged

feat(iotevents): add grant method to Input class #18617

merged 18 commits into from
Feb 3, 2022

Conversation

yamatatsu
Copy link
Contributor

This PR add grant method to Input class.
Next of this PR, I aim to create PR that add IoT Event Action to IoT Core Rule.

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 Jan 23, 2022

@github-actions github-actions bot added the @aws-cdk/aws-iotevents Related to AWS IoT Events label Jan 23, 2022
skinny85
skinny85 previously requested changes Jan 24, 2022
View reviewed changes
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 @yamatatsu! As always 🙂. Minor notes related mostly to docs.

packages/@aws-cdk/aws-iotevents/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iotevents/lib/input.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iotevents/lib/input.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iotevents/lib/input.ts Show resolved Hide resolved
packages/@aws-cdk/aws-iotevents/lib/input.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iotevents/test/input.test.ts Show resolved Hide resolved
packages/@aws-cdk/aws-iotevents/test/input.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iotevents/test/input.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iotevents/test/input.test.ts Show resolved Hide resolved
packages/@aws-cdk/aws-iotevents/test/input.test.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed skinny85’s stale review January 25, 2022 12:24

Pull request has been modified.

skinny85
skinny85 previously requested changes Feb 1, 2022
View reviewed changes
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.

Apologies I missed this @yamatatsu! A couple of minor notes/questions.

packages/@aws-cdk/aws-iotevents/lib/input.ts Outdated
* @attribute
*/
readonly inputName: string;

/**
* The ARN of the input
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The ARN of the input
* The ARN of the input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fix all same case in aws-iotevents 👍🏻

packages/@aws-cdk/aws-iotevents/lib/input.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iotevents/lib/input.ts Outdated
grantee,
actions,
resourceArns: [this.inputArn],
scope: this,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Why are we passing this argument here? I don't think it's required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. I copied from DynamoDB Table grant. I removed and confirm the test is passed.

packages/@aws-cdk/aws-iotevents/test/input.test.ts Show resolved Hide resolved
packages/@aws-cdk/aws-iotevents/test/input.test.ts Show resolved Hide resolved
packages/@aws-cdk/aws-iotevents/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iotevents/lib/input.ts Outdated
*
* @param grantee the principal
*/
grantPutMessage(grantee: iam.IGrantable): iam.Grant
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you actually think of renaming this to grantWrite()? Maybe that's a better name, and will allow us to expand the set of granted permissions here as needed in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sound's good!

will allow us to expand the set of granted permissions here as needed in the future?

That's reasonable!

Copy link
Contributor

Choose a reason for hiding this comment

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

So... why is this grantPushMessage() still? 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops..

yamatatsu and others added 2 commits February 2, 2022 11:59
@yamatatsu @skinny85
Update packages/@aws-cdk/aws-iotevents/lib/input.ts
4764a23 
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
@yamatatsu @skinny85
Update packages/@aws-cdk/aws-iotevents/lib/input.ts
aa3e2b8 
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
@mergify mergify bot dismissed skinny85’s stale review February 2, 2022 02:59

Pull request has been modified.

yamatatsu and others added 2 commits February 2, 2022 13:50
@yamatatsu @skinny85
Update packages/@aws-cdk/aws-iotevents/README.md
af4114e 
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
@yamatatsu
address comments
a8fc43d
skinny85
skinny85 previously requested changes Feb 2, 2022
View reviewed changes
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.

I think there are some merge conflicts with master that you need to solve @yamatatsu!

packages/@aws-cdk/aws-iotevents/lib/input.ts Outdated
*
* @param grantee the principal
*/
grantPutMessage(grantee: iam.IGrantable): iam.Grant
Copy link
Contributor

Choose a reason for hiding this comment

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

So... why is this grantPushMessage() still? 😛

@mergify mergify bot dismissed skinny85’s stale review February 3, 2022 01:27

Pull request has been modified.

skinny85
skinny85 approved these changes Feb 3, 2022
View reviewed changes
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 @yamatatsu, thanks for the contribution!

@mergify
Copy link
Contributor

mergify bot commented Feb 3, 2022

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: b9bc5a7
  • 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 e89688e into aws:master Feb 3, 2022
@mergify
Copy link
Contributor

mergify bot commented Feb 3, 2022

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

kornicameister added a commit to kornicameister/aws-cdk that referenced this pull request Feb 6, 2022
@kornicameister
Merge remote-tracking branch 'origin/master' into pr/18546
d098d5f 
* origin/master: (27 commits)
  chore(eks): deprecate older versions of EKS (aws#18842)
  fix(tooling): update vscode devcontainer image (aws#18455)
  chore: npm-check-updates && yarn upgrade (aws#18832)
  chore(docs): Fix broken md links (aws#18384)
  chore(lambda-layer-awscli): install awscli with pip and requirements.txt (aws#18800)
  fix(aws-appsync): Strip unsupported characters from Lambda DataSource (aws#18765)
  feat(cfnspec): cloudformation spec v55.0.0 (aws#18827)
  docs(cfnspec): update CloudFormation documentation (aws#18826)
  chore(cxapi): plugin context provider limited by cx schema (aws#18709)
  feat(iotevents): add grant method to Input class (aws#18617)
  chore(cx-api): break circular dependencies (aws#18767)
  docs(core): clarify that `addOverride` does not change property casing (aws#18687)
  feat(s3-deployment): deploy data with deploy-time values (aws#18659)
  docs(cfnspec): update CloudFormation documentation (aws#18808)
  feat(cli): `cdk diff` works for Nested Stacks (aws#18207)
  docs(cfnspec): update CloudFormation documentation (aws#18783)
  chore(lambda-layer-awscli): add update mechanism for AWS CLI (aws#18780)
  chore(release): 1.143.0
  feat(fsx): add support for FSx Lustre Persistent_2 deployment type (aws#18626)
  feat(amplify): support performance mode in Branch (aws#18598)
  ...
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
@yamatatsu @TikiTDO
feat(iotevents): add grant method to Input class (aws#18617)
3bb63db 
This PR add `grant` method to `Input` class.
Next of this PR, I aim to create PR that add IoT Event Action to IoT Core Rule.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@yamatatsu yamatatsu deleted the iotevents-input-grant branch April 6, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skinny85 skinny85 skinny85 approved these changes

Assignees

@skinny85 skinny85

Labels
@aws-cdk/aws-iotevents Related to AWS IoT Events
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yamatatsu @aws-cdk-automation @skinny85