add support for syncing Assessments from OneTrust to Transcend #376
Conversation
|
This pull request has been linked to and will mark 3 tasks as "Pending Deploy" when merged:
|
abrantesarthur
changed the title
abrantesarthur
requested review from
michaelfarrell76,
anotherminh and
a team
| hostname, | ||
| oneTrustAuth, | ||
| transcendAuth, | ||
| transcendUrl, |
There was a problem hiding this comment.
so transcendAuth + transcendUrl are optional? If they don't pass it in we'll just save to disk?
There was a problem hiding this comment.
Yes, with a caveat: the criteria for saving to disk is that --dryRun=true, in which case we the parseCliSyncOtArguments helper also ensures that transcendAuth + transcendUrl were passed in.
There was a problem hiding this comment.
Wait i thought dryrun=true would mean don't upload to Transcend
| /** | ||
| * fetch details about each assessment in series and write to transcend or to disk | ||
| * (depending on the dryRun argument) right away to avoid running out of memory | ||
| */ |
| * fetch details about each assessment in series and write to transcend or to disk | ||
| * (depending on the dryRun argument) right away to avoid running out of memory | ||
| */ | ||
| await mapSeries(assessments, async (assessment, index) => { |
There was a problem hiding this comment.
wish we could do this with some concurrency.. maybe chunks of 5-10 assessments at a time? IDK what their rate limit is but we'd want to stay under it
There was a problem hiding this comment.
I think OT would allow for it. My main concern was introducing concurrency bugs (e.g., writing to a file is pretty slow and it's possible to mess up the writing order if doing so concurrently).
There was a problem hiding this comment.
Perhaps we can do the fetching from OneTrust concurrently but the writing to Disk or Transcend synchronously? I. can try that.
There was a problem hiding this comment.
^ yea! that's sort of what i was thinking. Concurrently fire off 5-10 requests but then write it down concurrently
| const { risks, ...restQuestion } = question; | ||
| const enrichedRisks = (risks ?? []).map((risk) => { | ||
| const details = riskDetailsById[risk.riskId]; | ||
| // FIXME: missing the risk meta data and links to the assessment |
There was a problem hiding this comment.
did you mean to do this FIXME
| * @param param - information about the assessment and amount of entries | ||
| * @returns a stringified csv entry ready to be appended to a file | ||
| */ | ||
| export const oneTrustAssessmentToCsv = ({ |
There was a problem hiding this comment.
nit: would be nice to have some unit tests for these due to complexity but not a big deal
| | file | Path to the file to pull the resource into. Its format must match the fileFormat argument. | string | N/A | false | | ||
| | fileFormat | The format of the output file. | string | csv | false | | ||
| | resource | The resource to pull from OneTrust. For now, only assessments is supported. | string | assessments | false | | ||
| | dryRun | Whether to export the resource to a file rather than sync to Transcend. | boolean | false | false | |
There was a problem hiding this comment.
Is it worth trying to convey that some arguments are conditionally required? E.g. I assume if you have --dryRun then file is required, and if you don't then transcendAuth/transcendUrl are required
I suggest starting the review from the command's entrypoint
cli-sync-ot.ts.See updated README for instructions of how to test this. Locally, you'd do
yarn ts-node ./src/cli-sync-ot.ts. instead ofyarn cli-sync-ot.cli-pull-ot->cli-sync-ot.Note that requests to
importOneTrustAssessmentFormsfail because we need to update the endpoint, not because the cli has bugs.Related Issues
Security Implications
[none]
System Availability
[none]