Skip to content
This repository has been archived by the owner on Sep 9, 2022. It is now read-only.

add pagination to all resources #236

Merged
merged 1 commit into from
Jul 14, 2016
Merged

add pagination to all resources #236

merged 1 commit into from
Jul 14, 2016

Conversation

philsnow
Copy link
Contributor

The resources API has a super clean way of enumerating without tons of
boilerplate while resp.is_truncated loops.

I only did this for a few resource types because I wanted to check in with @dtan4 about using the resources API and didn't want to do the same work x20 if it's not mergeable.

@philsnow philsnow mentioned this pull request Jun 21, 2016
Closed
@coveralls
Copy link

coveralls commented Jun 21, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-1.9%) to 98.143% when pulling 9955468 on philsnow:philsnow/pagination into 48b6dc8 on dtan4:master.

@coveralls
Copy link

coveralls commented Jun 23, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling d3df8ee on philsnow:philsnow/pagination into 48b6dc8 on dtan4:master.

@dtan4
Copy link
Owner

dtan4 commented Jun 26, 2016

It looks good for IAM resources.

However, aws-sdk-resources gem currently supports a few resources (EC2, S3, IAM, SNS, SQS). So I'm worried that it would lose consistency between resources (e.g. IAM v.s. RDS)...

@philsnow
Copy link
Contributor Author

Oh thanks for pointing that out. I'll back this out and not use the
resources API.

On Sunday, June 26, 2016, Daisuke Fujita notifications@github.com wrote:

It looks good for IAM resources.

However, aws-sdk-resources gem currently supports a few resources (EC2,
S3, IAM, SNS, SQS)
https://github.com/aws/aws-sdk-ruby/tree/29af92571f27962ab50372badb9c7cc34909f529/aws-sdk-resources/lib/aws-sdk-resources/services.
So I'm worried that it would lose consistency between resources (e.g. IAM
v.s. RDS)...

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#236 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AASLNZZdvlIH01MEXVynm_RurkYBzA5Qks5qPrHMgaJpZM4I7RW7
.

@philsnow philsnow changed the title Use aws-sdk-resources to get pagination "for free" add pagination to iam_ resources Jun 27, 2016
@coveralls
Copy link

coveralls commented Jun 27, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-2.6%) to 97.359% when pulling edbf015 on philsnow:philsnow/pagination into 48b6dc8 on dtan4:master.

@philsnow philsnow force-pushed the philsnow/pagination branch from edbf015 to 7a9a8aa Compare June 27, 2016 10:03
@coveralls
Copy link

coveralls commented Jun 27, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 97.359% when pulling 7a9a8aa on philsnow:philsnow/pagination into e049e00 on dtan4:master.

@philsnow
Copy link
Contributor Author

PTAL @dtan4

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 97.359% when pulling e71a33d on philsnow:philsnow/pagination into e049e00 on dtan4:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jun 27, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 97.359% when pulling e71a33d on philsnow:philsnow/pagination into e049e00 on dtan4:master.

@philsnow
Copy link
Contributor Author

philsnow commented Jul 7, 2016

ping @dtan4

Does this look okay for the resources I added it to ? I will finish with the other resources but I want to hear what you think about the approach first.

dtan4
dtan4 reviewed Jul 7, 2016
View reviewed changes
lib/terraforming/version.rb
@@ -1,3 +1,3 @@
module Terraforming
VERSION = "0.9.1"
VERSION = "0.10.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Please do not bump the version in this pull request.

If you'd like to install this patch locally, just run rake install.

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, and didn't know about rake install, thanks

@dtan4
Copy link
Owner

dtan4 commented Jul 7, 2016

I'm sorry for late response... I missed your ping.

As described here, fetching all resources can be written more simply. I prefer this style rather than the logic with Proc.

client.list_users.map(&:users).flatten

Is there any strong reason to use Proc?

dtan4
dtan4 reviewed Jul 7, 2016
View reviewed changes
lib/terraforming/resource/iam_policy_attachment.rb
data_proc = Proc.new { |resp| [resp] }
paginate_results(fetch_proc, data_proc).each do |resp|
require 'pp'
STDERR.puts resp.pretty_inspect
Copy link
Owner

Choose a reason for hiding this comment

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

These two lines seems to be inserted for debugging.
Please make sure that commit does not contain any debugging code :octocat:

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, sorry

@coveralls
Copy link

coveralls commented Jul 7, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 97.346% when pulling 1cc15b3 on philsnow:philsnow/pagination into e049e00 on dtan4:master.

dtan4
dtan4 reviewed Jul 8, 2016
View reviewed changes
spec/lib/terraforming/resource/iam_group_spec.rb
'id' => 'fuga',
'attributes' => {
'arn' => 'arn:aws:iam::345678901234:group/fuga',
'id' => 'fuga',
Copy link
Owner

Choose a reason for hiding this comment

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

[rubocop]

  • Align the elements of a hash literal if they span more than one line.

@coveralls
Copy link

coveralls commented Jul 8, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 97.347% when pulling bfa266a on philsnow:philsnow/pagination into e049e00 on dtan4:master.

@dtan4
Copy link
Owner

dtan4 commented Jul 8, 2016

@philsnow I'm sorry but you do not have to fix all warnings by RuboCop now. If you would merge master into this branch now, most of warnings might be suppresed.

@philsnow philsnow force-pushed the philsnow/pagination branch from bfa266a to 1352e5f Compare July 8, 2016 15:27
dtan4
dtan4 reviewed Jul 8, 2016
View reviewed changes
lib/terraforming/resource/iam_group_membership.rb
@@ -44,11 +44,11 @@ def tfstate
private

def group_members_of(group)
@client.get_group(group_name: group.group_name).users.map { |user| user.user_name }
@client.get_group(group_name: group.group_name).map(&:users).flatten.map { |user| user.user_name }
Copy link
Owner

Choose a reason for hiding this comment

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

[rubocop]

  • Line is too long. [106/80] :ref

@coveralls
Copy link

coveralls commented Jul 8, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 97.346% when pulling 1352e5f on philsnow:philsnow/pagination into 8bc3835 on dtan4:master.

dtan4
dtan4 reviewed Jul 8, 2016
View reviewed changes
lib/terraforming/resource/route53_zone.rb
@@ -49,7 +49,7 @@ def tfstate
private

def hosted_zones
@client.list_hosted_zones.hosted_zones.map { |hosted_zone| @client.get_hosted_zone(id: hosted_zone.id) }
@client.list_hosted_zones.map(&:hosted_zones).flatten.map { |hosted_zone| @client.get_hosted_zone(id: hosted_zone.id) }
Copy link
Owner

Choose a reason for hiding this comment

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

[rubocop]

  • Line is too long. [127/80] :ref

@coveralls
Copy link

coveralls commented Jul 8, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.009%) to 97.344% when pulling efe3a6c on philsnow:philsnow/pagination into 8bc3835 on dtan4:master.

@philsnow
Copy link
Contributor Author

philsnow commented Jul 8, 2016

which rubocop warnings do you care about? I see that there's a rubocop.yml file, but I rebased my PR against master and a lot of the things it's complaining about are 1. also warnings in master and 2. would make the code not look like the surrounding code.

@dtan4
Copy link
Owner

dtan4 commented Jul 11, 2016

I'd like to care all warnings RuboCop said, however I'm in the middle of configuring RuboCop to detect the appropriate warnings. For example I should relax the threshold of line length from now on.

I'm so sorry you're in trouble with unexpected RuboCop warnings. Most of warnings appeared currently are not related to your changes. So you can ignore all warnings now and I'll fix these on another pull request by myself.

@dtan4
Copy link
Owner

dtan4 commented Jul 11, 2016

Ah, you added pagination to all resources at efe3a6c.
Thank you for your work, but please rename this pull request to add pagination to all resources please.

After that, I'll merge this pull request.

@philsnow philsnow force-pushed the philsnow/pagination branch from efe3a6c to 2e55bee Compare July 12, 2016 23:49
@philsnow philsnow changed the title add pagination to iam_ resources add pagination to all resource types Jul 12, 2016
@philsnow
Copy link
Contributor Author

ok also squashed commits, there wasn't anything interesting in the history.

@coveralls
Copy link

coveralls commented Jul 12, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.009%) to 97.344% when pulling 2e55bee on philsnow:philsnow/pagination into 8bc3835 on dtan4:master.

@philsnow philsnow changed the title add pagination to all resource types add pagination to all resources Jul 13, 2016
@dtan4
Copy link
Owner

dtan4 commented Jul 14, 2016

LGTM 👍 Thank you.

@dtan4 dtan4 merged commit 6ae7c37 into dtan4:master Jul 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@philsnow @coveralls @dtan4