Skip to content
This repository has been archived by the owner on Feb 22, 2024. It is now read-only.

Update historical-requests for new S3 location #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

buckett
Copy link
Contributor

@buckett buckett commented Feb 14, 2023

Thanks for submitting a PR! We want to make contributing to the Canvas Data CLI as easy as possible.
Please read these instructions carefully:

  • Explain the motivation for making this change.
  • Provide a test plan demonstrating that the code is solid.
  • Match the code formatting of the rest of the codebase.
  • Make sure to add tests to help keep code coverage up.

Motivation (required)

Fixes the historical-requests subcommand to work with the new file location in S3.

Test Plan (required)

Run canvasDataCli historical-requests and check that it groups files by date range.

Next Steps

  • Small pull requests are much easier to review and more likely to get merged. Make sure the PR does only one thing, otherwise please split it.
  • Make sure all tests pass, we will run this on jenkins but you can run it yourself with the build.sh script.

At some point in the past Instructure changed the path of the files on S3 and this broke the assumptions in the tool about where the timestamp/daterange is contained. This updates the code to use the newer location.

@dtod Correctly suggested the fix that was needed.

fixes: 40
@buckett
Copy link
Contributor Author

buckett commented Feb 14, 2023

This should fix #40

@buckett
Copy link
Contributor Author

buckett commented Feb 14, 2023

Ok, but it's not that simple as the newer requests are of the format:

https://s3.amazonaws.com/<timestamp>/dw_split/<account>/requests/b%3D1/part-<uuid>.c001.txt.gz

so the code should cope with both options. This does mean you either get a range or a timestamp in the grouping.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant