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

Add MessageId to message object as id #24

Merged
merged 3 commits into from
Jun 23, 2020
Merged

Add MessageId to message object as id #24

merged 3 commits into from
Jun 23, 2020

Conversation

kevin-yen
Copy link
Contributor

@kevin-yen kevin-yen commented Jun 22, 2020 •
edited
Loading

Background

I have a use case where I need the message ID of a particular message. This PR adds MessageId to the list of fields returned by the message object

Changes

  • Add MessageId to the object returned by sqs_receive_message as the field id

@mattBrzezinski
Copy link
Member

The CI has been having some issues it seems, I will try and resolve those some time this week.

These changes look good to me, would you be able to add in a small test for this as well?

@kevin-yen
Copy link
Contributor Author

Not yet. I'll go ahead and add some tests now

@kevin-yen
Copy link
Contributor Author

@mattBrzezinski I added a check that message[:id] exists and is not an empty string. Is that good enough or would you want to match the id with the one that's sent? To do that I think we will need access to the response of sqs_send_message. I'm not sure whether we're reading right now

mattBrzezinski
mattBrzezinski reviewed Jun 23, 2020
View reviewed changes
test/queue.jl Outdated Show resolved Hide resolved
@mattBrzezinski
Copy link
Member

Awesome, this looks good to me!

I'm going to try and dedicate some time to fix the CI before merging this in. I'm hoping to have some time tomorrow to resolve the issues with it.

@mattBrzezinski
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jun 23, 2020
@bors
Try #24:
e9aaa32
@bors
Copy link
Contributor

bors bot commented Jun 23, 2020

try

Build succeeded:

@mattBrzezinski
Copy link
Member

I believe that the CI is fixed, could you rebase on the latest master?

@kevin-yen
Copy link
Contributor Author

Rebased

@mattBrzezinski
Copy link
Member

I'm going to use this PR as a guinea pig. I'm currently unable to set bors as a check requirement before merging.

I believe it's because this PR was created before my CI changes in #26. I'm just going to close this PR, then re-open it, and see if it adds bors to the list of checks we can use.

@mattBrzezinski
Copy link
Member

bors r+

@mattBrzezinski
Copy link
Member

Looks like that worked!

@bors
Copy link
Contributor

bors bot commented Jun 23, 2020

Build succeeded:

@bors bors bot merged commit eea6d03 into JuliaCloud:master Jun 23, 2020
@kevin-yen kevin-yen deleted the add-message-id branch June 23, 2020 20:27
@mattBrzezinski mattBrzezinski mentioned this pull request Jun 23, 2020
Merged
bors bot added a commit that referenced this pull request Jun 23, 2020
@bors @mattBrzezinski
Merge #28
98e1b6b 
28: Version bump r=iamed2 a=mattBrzezinski

Release a new version including:

#21: Add sqs_change_message_visibility function
#24: `sqs_receive_message` returns back the message `id` as well

Co-authored-by: Matt Brzezinski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattBrzezinski mattBrzezinski mattBrzezinski left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kevin-yen @mattBrzezinski