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

Fix Barbeque::SnsSubscriptionsController#fetch_sns_topic_arns #70

Merged
merged 1 commit into from
Jul 13, 2018

Conversation

itkq
Copy link
Contributor

@itkq itkq commented Jul 3, 2018

Aws::SNS::Client#list_topics returns up to 100 topics. This
change allows to fetch all topics when there are more than 100
topics.

@itkq itkq force-pushed the fix-fetch-sns-topic-arns branch from efac99d to 141faf3 Compare July 3, 2018 08:53
next_token = nil

loop do
list_topics = Barbeque::SNSSubscriptionService.sns_client.list_topics(next_token: next_token)
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI https://github.com/aws/aws-sdk-ruby#paging-responses

To make it easy to get the next page of results, every AWS response object is enumerable:

@itkq itkq force-pushed the fix-fetch-sns-topic-arns branch from 141faf3 to 780fcc0 Compare July 3, 2018 09:56
@riseshia
Copy link
Contributor

riseshia commented Jul 3, 2018

cc @cookpad/dev-infra

end

def each_sns_topic_arns
unless block_given?
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any reason you want to support passing block?

@itkq itkq force-pushed the fix-fetch-sns-topic-arns branch from 780fcc0 to 83d93e1 Compare July 3, 2018 10:09
@@ -44,6 +44,6 @@ def destroy
private

def fetch_sns_topic_arns
Barbeque::SNSSubscriptionService.sns_client.list_topics.topics.map(&:topic_arn)
Barbeque::SNSSubscriptionService.sns_client.list_topics.flat_map{ |resp| resp.topics.map(&:topic_arn) }
Copy link

@ganmacs ganmacs Jul 3, 2018

Choose a reason for hiding this comment

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

This is enough, isn't it.
Barbeque::SNSSubscriptionService.sns_client.list_topics.flat_map(&:topics).map(&:topic_arn)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. b28f812

@itkq itkq force-pushed the fix-fetch-sns-topic-arns branch from 83d93e1 to b28f812 Compare July 3, 2018 14:54
@riseshia
Copy link
Contributor

riseshia commented Jul 3, 2018

LGTM

@riseshia riseshia merged commit a558725 into cookpad:master Jul 13, 2018
@itkq itkq deleted the fix-fetch-sns-topic-arns branch July 14, 2018 06:22
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.

None yet

4 participants