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

Export endpoint for ds-s #68

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Conversation

ebefarooqui
Copy link
Contributor

This PR creates an endpoint /api/export which retrieves all resources in the sheet and writes them to a singular file specified by the env var EXPORT_FILE_DEST

The data in the file is a JSON object indexed like the following:


{
   'url_resource': data,
   'url_resource_1': data,
   'url_resource_2': data,
   ...
}

Ping me and I'll send you an example file

@breezykermo
Copy link
Member

Is there a known reason that the CI hangs on npm test for these changes? Shouldn't it not affect those tests, or were they already broken in some way?

Copy link
Member

@breezykermo breezykermo left a comment

Choose a reason for hiding this comment

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

This LGTM. We also talked at one point about some runtime parameter when starting up datasheet-server (i.e. npm run dev -- --headless) which bypasses the loop that keeps the server available, and just pulls the data, saves it to disk, and then completes. It would make sense to me if that logic were added to this PR, along with a modification to the test suite that checks it works as expected when passed the flag.

src/api/index.js Outdated
@@ -4,6 +4,7 @@ import copy from '../copy/en'

export default ({ config, controller }) => {
let api = Router()
const fileDest = config.EXPORT_FILE_DEST || ""
Copy link
Member

Choose a reason for hiding this comment

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

What is the logic behind the default here being an empty string? Would it be better as 'data_export.json' or something similarly generic?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see down below that the empty string is being used as a 'null' value. Maybe we explicitly use null instead?

@zacoppotamus
Copy link

Nice work @ebefarooqui, this looks good to me code-wise. I can't seem to export when running EXPORT_FILE_DEST=out.json && npm run dev and hitting /api/export. I saw that EXPORT_FILE_DEST is expected to be a folder and made it so but it still doesn't work.

To echo @breezykermo's comment, personally I would find it very useful if we were able to invoke datasheet as a globally installed-CLI tool (eg @forensic-architecture/datasheet). Maybe we could think about what parameters we could use to either spin up a server with a provided config file, or alternatively parse and export the JSON file.

@ebefarooqui
Copy link
Contributor Author

Is there a known reason that the CI hangs on npm test for these changes? Shouldn't it not affect those tests, or were they already broken in some way?

@ebefarooqui
Copy link
Contributor Author

Didn't mean to close the above ^

@ebefarooqui ebefarooqui reopened this Feb 13, 2021
@ebefarooqui
Copy link
Contributor Author

@zacoppotamus I'm in favor of trying to modify ds-s to be a CLI-esque tool that is dynamic and compatible with these exported json files. This I feel would exist as an alternative way of using ds-s but wouldnt necessarily require rehashing everything. I'm going to pull down and see if everything checks out, not sure why youre having that issue. Could be because the server is looking for the config and .env files and they might not be in the right locations? We'll see

@ebefarooqui
Copy link
Contributor Author

@breezykermo I can take a look at making the logic run as an npm script and adding that parameterization accordingly.

@breezykermo
Copy link
Member

I think the startup for datasheet already runs via npm scripts, npm start in production from memory. The headless command could even (for now) just be a separate items in npm scripts, i.e. npm run headless in order to get the scriptive functionality.

@ebefarooqui
Copy link
Contributor Author

I think the startup for datasheet already runs via npm scripts, npm start in production from memory. The headless command could even (for now) just be a separate items in npm scripts, i.e. npm run headless in order to get the scriptive functionality.

I initially went about it this way but wanted to expose an endpoint for researchers to use as well instead of having to run it as an npm script. I could write an additional npm script as well, not opposed.

@ebefarooqui
Copy link
Contributor Author

@zacoppotamus I believe you need to run the command as so EXPORT_FILE_DEST='path' npm run dev (ie. nix the ampersands)

@zacoppotamus
Copy link

@ebefarooqui thanks Ebe, works great.

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.

3 participants