feat(iot): add Action to forward message attributes to Location #24060
Conversation
|
|
github-actions
bot
added
beginning-contributor
There was a problem hiding this comment.
Thank you for your contribution! Overall this looks great, I just have some comments inline.
For a second opinion, @yamatatsu, you've been leading the charge on IoT modules, could you take a look through this and provide feedback as well? I think you understand the service much better than I do.
packages/@aws-cdk/aws-iot-actions/test/location/integ.location-action.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-iot-actions/test/location/location-action.test.ts
Outdated
Show resolved
Hide resolved
mergify
bot
dismissed
TheRealAmazonKendra’s stale review
Pull request has been modified.
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
@yamatatsu would be great if we could merge this soon so that I don't have to update the PR after another refactor again |
There was a problem hiding this comment.
@TheRealAmazonKendra
Thanks for the mention!! And so sorry for the delay 🙇🏻.
@sthuber90
Thanks for the re-mention!! 🙏🏻
I have made some comments including minor style comments, learning from comments I have received from maintainers in the past😉.
| Timestamp: this.timestamp | ||
| ? { Value: this.timestamp.value, Unit: this.timestamp.unit } | ||
| : { | ||
| Value: '${timestamp()}', | ||
| Unit: LocationTimestampUnit.MILLISECONDS, | ||
| }, |
There was a problem hiding this comment.
Timestamp has the default behabior defined by AWS as written in CFn documentation:
The default value is the time the MQTT message was processed.
So I would prefer not to define the cdk original default behabior. Otherwise, if user want to use the default behaior, the user will have to set as Timestamp: {}.
There was a problem hiding this comment.
Hope I moved this more into the direction that you envisioned. Hope to get rid of this override soon
| const topicRule = rule.node.defaultChild as CfnTopicRule; | ||
| topicRule.addOverride('Properties.TopicRulePayload.Actions.0', { |
There was a problem hiding this comment.
This workaround will probably not work when using this action for second action of TopicRile because of Actions.0.
But I don’t know alternative…
There was a problem hiding this comment.
It is maybe difficult to implement this action without fixing this issue I think...
There was a problem hiding this comment.
Fully agree with you but also couldn't come up with a better workaround
There was a problem hiding this comment.
Thank you very much for your review. I really appreciate all your feedback
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
1 similar comment
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
| * @param tracker The Amazon Location tracker to which to write data. | ||
| * @param props Optional properties to not use default | ||
| */ | ||
| constructor(private readonly tracker: location.CfnTracker, props: LocationActionProps) { |
There was a problem hiding this comment.
We don't want to expose any L1s in an L2. For this PR we will need to implement an L2 for CfnTracker as well.
There was a problem hiding this comment.
I will try to add a L2 for the Location Tracker but will that be ok? The contribution guideline says that a new L2 must reference a RFC
There was a problem hiding this comment.
It looks like location tracker is a pretty simple resource and we already have an alpha module for location so we can skip the RFC.
aws-cdk-automation
added
the
pr/needs-community-review
corymhall
removed
the
pr/needs-community-review
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Aims to add IoT action to integrate with Location service through L2 construct.
The construct depends on
CfnTrackerfrom aws_location. I tried to import it withimport { CfnTracker } from '@aws-cdk/aws-location'but without the following exclude in package.jsonyarn buildfailed.{ // ... "pkglint": { "exclude": [ "no-experimental-dependencies" ] } }According to the docs the location module is not experimental but there is a location-alpha module. So any info on how I need to import the location module to get rid of the include is greatly appreciated.
Closes #24035.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license