-
Notifications
You must be signed in to change notification settings - Fork 150
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
Fetch all entries to transform #83
Fetch all entries to transform #83
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is great 👏
If possible I'd like to see some tests added to https://github.com/contentful/migration-cli/blob/master/test/unit/lib/fetcher.spec.ts which currently doesn't seem to cover entry fetching at all. 😞
Just a quick one where the first request returns one entry, and then the second request also returns one entry with a total of two.
That of course requires skip
to be using response.items.length
.
src/lib/fetcher.ts
Outdated
}) | ||
|
||
entries = entries.concat(response.items) | ||
skip += 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to have a magic number here and use response.items.length
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
@TimBeyer I'll add some tests after lunch. Update: done ✅ |
LGTM although at some point we will change the code to not have all entries in memory at once. |
@connor-baer Thanks for the PR we'll release a |
Fixes #82.