Create JSON files instead of CSV from Collectors #63
Conversation
Instead of having each fetcher return a CSV string, this update allows each fetcher to create it's own data files directly. This should make things more open for extension in the future, because a fetcher doesn't need to be tied to a specific return value. Instead, it can format files directly however it likes (or append to multiple files if needed). Co-authored-by: Andrew Henry <[email protected]>
When we build the next site, we can automatically include the static data so that we don't need to check the data into the source code. This updates the workflow to build the JSON file before the rest of the site so it's included in the frontend build. Co-authored-by: Andrew Henry <[email protected]>
Because the build step now includes the data files, we don't need a separate action to do those. We can instead remove this and depend on the nextjs workflow to automatically load the data it needs. If we need CSV functionality, we can either 1) write an action/workflow to download a CSV for you or 2) add a CSV download button to the UI. Co-authored-by: Andrew Henry <[email protected]>
Allows us to set the organization name for fetching data via an environemnt variable. Left a fallback in place so that this can be optional for now during development. Also removed the csv/json flag as we ended up not using that pattern.
|
Just tested this again after updating form main and got an error: |
|
The issue I had was just with my PAT - this is ready for review! |
| @@ -2,10 +2,13 @@ | |||
| const nextConfig = { | |||
| reactStrictMode: true, | |||
| productionBrowserSourceMaps: true, | |||
| basePath: '/sbv-world-health-org-metrics', | |||
| basePath: | |||
There was a problem hiding this comment.
Thanks for adding this! Drove me crazy trying to figure out why I couldn't get the pages to load my first time spinning up the front end
backend/business/core/collectors/github/fetchers/repos_info_fetcher.go
Outdated
Show resolved
Hide resolved
- Remove formatCSV functions, which are now unused - Rename on instnace of `buildJSON` to `buildResult` for consistency Co-authored-by: David Gardiner <[email protected]>
#66 contains the relevant information for adding this feature.
|
Confirmed that the build was successful once updating the environment variables: https://github.com/sbv-world-health-org-metrics/sbv-world-health-org-metrics/actions/runs/6787581045 (the deploy failed because of a failed protection rule - interestingly the rule looks like deployments from any branch should be allowed 🤷). I'm going to go ahead and merge and see if the deploy succeeds afterwards. |
@ajhenry and I have been working on adding the data generated by the backed into the frontend application. To better integrate with the proposed frontend, this PR overhauls the
Fetcherinterface. Instead of having each Fetcher return a CSV string, we instead have the fetchers return structs or maps that hold data from their particular endpoint. TheCollectorthen takes that data and generates a JSON file which can be consumed by the frontend application.This also updates the next.js build action so that we generate the JSON files as part of the build step. Each time we re-deploy the app, we'll want the ability to re-populate the dataset.
There may be a world where we want to make the data-fetching optional (so that, for example, we could still deploy UI updates if the GitHub API was down), but we didn't worry too much about that for now.
Would love a thorough review on this since it's a relatively big change - definitely open to other suggestions or feedback.