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

Note "Subject" is not optional for SNS messages #315

Closed
wants to merge 1 commit into from

Conversation

sgillies
Copy link

@sgillies sgillies commented Aug 6, 2019

@sgillies sgillies requested review from rclark and drboyer August 6, 2019 20:55
Copy link
Contributor

@vsmart vsmart left a comment

Choose a reason for hiding this comment

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

Thanks @sgillies.

Can you fix the travis failure?

@@ -27,7 +27,8 @@

### 4.13.0

- Adds support for first-in-first-out (FIFO) SQS queues. https://github.com/mapbox/ecs-watchbot/pull/279
- Adds support for first-in-first-out (FIFO) SQS queues. https://github.com/mapbox/ecs-watchbot/pull/279.
- SNS message objects are required to have both a "Message" and a "Subject" property. Previously, "Subject" was optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you link to the issue ticket here? This wording sounds like it was an intentional change, but in fact it was a regression.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just so I understand, subjects used to be required, a regression was introduced at some point and it was fixed at version 4.13.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

No. Subject used to be optional. Now they are required, i.e. the regression is that we accidentally introduced this requirement. More details at #304.

Copy link
Author

@sgillies sgillies Aug 22, 2019

Choose a reason for hiding this comment

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

@vsmart the Travis errors were transient, I re-ran the builds and all is green.

Point taken about this being more of a regression. I don't usually edit my own change logs to retroactively note regressions introduced, only their fixes. Feel free to close this, though I would love to see some action. 3 different Mapbox projects stumbled over this.

Me and my team are fine about Subject being required, by the way. We went without them originally because we didn't know any better.

Copy link
Contributor

@drboyer drboyer left a comment

Choose a reason for hiding this comment

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

I wonder if there's an appropriate place to capture this in the repo documentation. I fear only mentioning this in the changelog might still be a bit buried.

Perhaps either a note about message format in the README (could crib some of the wording from #279?) or in the "Worker Runtime Environment" doc?

@sgillies sgillies closed this Dec 24, 2019
@sgillies sgillies deleted the subject-is-not-optional branch December 24, 2019 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants