-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix tr-pull-ot cli #383
fix tr-pull-ot cli #383
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.
I had a few questions but overall lgtm
@@ -45,6 +45,7 @@ export const enrichOneTrustAssessment = ({ | |||
return { | |||
...risk, | |||
...details, | |||
level: risk.level, |
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.
Does ...risk
not set level already? Or is it getting clobbered by ...details
?
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.
It was getting clobbered.
|
||
// Add comma for all items except the last one | ||
const comma = index < total - 1 ? ',' : ''; | ||
const comma = index < total - 1 && !wrap ? ',' : ''; |
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 swear I saw a different PR recently that also fixed this problem--did that one not go in? Or did the same bug exist in multiple places? (mostly asking because if it exists in multiple places, it might be worth doing a more general search to see if you can find any other instances of it)
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.
Great catch! it got merged with https://github.com/transcend-io/cli/pull/382/files. I forgot to merge dev.
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.
Missing some context again, but lgtm!
@@ -24,13 +24,13 @@ export const oneTrustAssessmentToJson = ({ | |||
let jsonEntry = ''; | |||
// start with an opening bracket | |||
if (index === 0 || wrap) { |
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 see the comment for wrap
says Whether to wrap every entry in brackets
, and it looks like we want to do that only when importing to transcend (mimicking a CSV)... but what is the syncOneTrustAssessmentToDisk
option?
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.
When using syncOneTrustAssessmentToDisk
, the output is [{},{},{}...]
(i.e., the first item starts with [
and the last ends with `]). Does that answer your question?
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.
No, I actually meant what is the difference between sync to transcend and sync to disk.
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.
Oh, sync to disk just means writing the pulled data to a json file.
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.
Are they both steps in the import process or are they two unique import options?
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.
pulled data to a json file.
Because this is distinct from the risk files that get uploaded to s3, right? Would we ever want an imported assessment in JSON and not in Transcend? If it is in Transcend, why do we need it in JSON? Just for a historical record?
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.
We, as Transcend, would always want to sync to Transcend. However, the cli is also available for customer use, and they may find value in keeping the records in Json as well.
enrichOneTrustAssessment
andoneTrustAssessmentToJson
helpers to correctly format the json into a shape accepted by theimportOneTrustAssessmentForms
mutation.Related Issues
Security Implications
[none]
System Availability
[none]