feat: bus stack #55
Merged
feat: bus stack #55
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0a48efb to
bebc39c
Compare
bebc39c to
23f5877
Compare
23f5877 to
3a15987
Compare
3a15987 to
0aa0f34
Compare
View stack outputs
|
0aa0f34 to
67cf627
Compare
67cf627 to
8a47303
Compare
8a47303 to
ded01b5
Compare
ded01b5 to
68bb324
Compare
68bb324 to
148a0db
Compare
148a0db to
2474edc
Compare
2474edc to
e16b705
Compare
e16b705 to
562d840
Compare
562d840 to
e00c697
Compare
e00c697 to
de7a4de
Compare
de7a4de to
5e22a6b
Compare
5e22a6b to
ed8add2
Compare
olizilla
reviewed
Nov 30, 2022
stacks/carpark-stack.js
Outdated
| target: { | ||
| message: awsEvents.RuleTargetInput.fromObject({ | ||
| region: awsEvents.EventField.fromPath('$.detail.region'), | ||
| bucket: awsEvents.EventField.fromPath('$.detail.bucketName'), |
There was a problem hiding this comment.
Any reason not to match the original property name here? I am wary of renaming properties across events as it makes it harder to guess what a properties name will be when coding handlers for them.
Suggested change
| bucket: awsEvents.EventField.fromPath('$.detail.bucketName'), | |
| bucketName: awsEvents.EventField.fromPath('$.detail.bucketName'), |
olizilla
approved these changes
Nov 30, 2022
carpark/utils/parse-sqs-event.js
Outdated
|
|
||
| return { | ||
| bucketRegion, | ||
| bucketName, | ||
| bucketRegion: region, |
There was a problem hiding this comment.
discussion: same here... region becomes bucketRegion... can we normalise to avoid the renames?
satnav/event-bus/source.js
Outdated
| * @param {string} eventBusName | ||
| */ | ||
| export async function notifyBus(event, eventBridge, eventBusName) { | ||
| const entries = event.Records |
There was a problem hiding this comment.
Suggested change
| const entries = event.Records | |
| const s3Events = event.Records |
satnav/event-bus/source.js
Outdated
Comment on lines
21
to
29
| if (entries.length > 0) { | ||
| const feedbackEntries = entries.map((entry) => ({ | ||
| EventBusName: eventBusName, | ||
| Source: SATNAV_EVENT_BRIDGE_SOURCE_EVENT, | ||
| DetailType: 'satnav_index_added', | ||
| Detail: JSON.stringify(entry), | ||
| })) | ||
| await eventBridge.putEvents({ Entries: feedbackEntries }) | ||
| } |
There was a problem hiding this comment.
minor: i think of this as mapping s3 events to bus events so suggestion is to name the lists to suggest that to the reader
Suggested change
| if (entries.length > 0) { | |
| const feedbackEntries = entries.map((entry) => ({ | |
| EventBusName: eventBusName, | |
| Source: SATNAV_EVENT_BRIDGE_SOURCE_EVENT, | |
| DetailType: 'satnav_index_added', | |
| Detail: JSON.stringify(entry), | |
| })) | |
| await eventBridge.putEvents({ Entries: feedbackEntries }) | |
| } | |
| if (s3Events.length > 0) { | |
| const busEvents = s3Events.map((entry) => ({ | |
| EventBusName: eventBusName, | |
| Source: SATNAV_EVENT_BRIDGE_SOURCE_EVENT, | |
| DetailType: 'satnav_index_added', | |
| Detail: JSON.stringify(entry), | |
| })) | |
| await eventBridge.putEvents({ Entries: busEvents }) | |
| } |
satnav/test/event-bus.test.js
Outdated
|
|
||
| const eventBusName = 'event-bus-arn' | ||
|
|
||
| test('notifies event bus when new satnav bucket is written', async t => { |
There was a problem hiding this comment.
Suggested change
| test('notifies event bus when new satnav bucket is written', async t => { | |
| test('notifies event bus when an .idx file is added to the satnav bucket', async t => { |
satnav/test/event-bus.test.js
Outdated
| t.is(response.statusCode, 200) | ||
| }) | ||
|
|
||
| test('does not notify event bus when satnav bucket is written with non idx files', async t => { |
There was a problem hiding this comment.
Suggested change
| test('does not notify event bus when satnav bucket is written with non idx files', async t => { | |
| test('does not notify event bus when a non .idx file is added to the satnav bucket', async t => { |
| const carId = sqsEvent.Records[0].body | ||
| const info = carId.match(/([^/]+)\/([^/]+)\/(.+)/) | ||
| if (!info) { | ||
| const body = sqsEvent.Records[0].body |
There was a problem hiding this comment.
do we end up with a lib module at some point? I wish we didn't have to do this boilerplate at all!
There was a problem hiding this comment.
Yes, I think we should. I will create issue
| @@ -22,7 +23,9 @@ export function SatnavStack({ stack }) { | |||
| const stackConfig = getConfig(stack.stage) | |||
|
|
|||
| // Get carpark reference | |||
| const { carparkBucket, carparkEventBus } = use(CarparkStack) | |||
| const { carparkBucket } = use(CarparkStack) | |||
There was a problem hiding this comment.
I wonder if we can limit the permissions here so that the index writer can only read files from the bucket.
There was a problem hiding this comment.
We should look into overall permissions yes. I will create issue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds bus stack with global custom EventBridge (future will also include Websocket API).
As part of this refactor to use Bus Stack, I also looked into removing the need to have a lambda function on the Event Bridge exit just to transform S3Event into a SQS Topic. A new transformation target rules was created to achieve this. Initially, I intended to keep same structure as what E-IPFS indexer receives (
${message.region}/${message.bucketName}/${message.key}), so that we could also remove that handler. This ended up not working in any way with AWS Events Rules + InputTransformerProperty + RuleTargetInput. Ended up just relying on a new JSON string with the expected properties, which also is nicer because we don't need to apply regexes to remove things from the string as before. We still need to handle E-IPFS indexing with a lambda, but that interaction will also change in the near future.This was tested with deployed resources and everything is working as expected - CAR uploaded to CARPARK triggers lambda to copy, and
*.idxfile is written into SATNAV bucketOther notes:
event-bridge/index.jsfile toevent-bridge/source.jsto better represent what that function does. It is the source event for the event bridge (so, lambda that forwards S3 Put Event to the Event bridge)Closes #53