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

Implement pagination using auto_paginate #13

Merged
merged 8 commits into from
Feb 5, 2022
Merged

Implement pagination using auto_paginate #13

merged 8 commits into from
Feb 5, 2022

Conversation

duncan-bayne
Copy link
Contributor

@duncan-bayne duncan-bayne commented Jan 31, 2022

It turns out there's no point in manually paginating; the Oktokit docs recommend the use of auto_paginate instead.

This PR changes to using auto_paginate, and introduces an integration test with HTTP record and replay courtesy VCR. In addition to the automated integration test, I've manually verified this against a test account with 200 EOL Ruby repositories in it.

I'm not sure where the upper bound lies for auto-pagination :)

It also:

  • Adds some development dependencies to the Gemspec.
  • Bumps Octokit to 4.22.
  • Adds style checks with standard to the default Rake task.

Closes #9.

It turns out there's no point in manually paginating; the Oktokit docs
recommend the use of auto_paginate instead.

This commit changes to using auto_paginate, and introduces an
integration test with HTTP record and replay courtesy VCR.
end_of_life.gemspec Outdated Show resolved Hide resolved
end_of_life.gemspec Outdated Show resolved Hide resolved
@duncan-bayne
Copy link
Contributor Author

Will make the above changes soon. I'll also have a read through the let post you linked; last I was doing Ruby for a living it was all the rage, but not without controversy :)

Also the VCR-based integration test is failing on Ruby 2.7, 3, and 3.1. "But it works on my machine" 🤦 I'll look into that failure too.

RSpec.describe EndOfLife::Repository do
describe "#fetch" do
it "fetches all 200 repositories from an account despite exceeding page size" do
repositories = VCR.use_cassette("many_repositories") do
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like this is not using the VCR cassette. Make sure to commit the cassettes if you want CI to use them.

Be sure that the cassettes don't leak your GITHUB_TOKEN. If they do, we can just rely on mocks, instead.

Copy link
Contributor Author

@duncan-bayne duncan-bayne Feb 4, 2022

Choose a reason for hiding this comment

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

Ah! Got it. Amazing what a few hours of good sleep can do ...

It was using the cassette, and the cassette was committed, but the check for the presence of GITHUB_TOKEN happens way before the cassette is read. The fix is to set the environment variable in the spec.

Thanks for the warning re. leaking real tokens. I had already taken care of this with the following VCR configuration:

config.filter_sensitive_data("REDACTED") { ENV["GITHUB_TOKEN"] }

Note that I'm setting this to the REDACTED value expected by config.

Also, this used to work locally - I must have accidentally killed the
`with_env` line while rebasing against main.
* Old hash syntax
* Redundant variable assignment
In addition to running specs, the default Rake task now also performs
style checking with standard.
@MatheusRich
Copy link
Owner

Awesome! Thanks for the contribution, @duncan-bayne! 🎉

@MatheusRich MatheusRich merged commit 6dcce58 into MatheusRich:main Feb 5, 2022
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.

Handle incomplete results from GitHub API
2 participants