-
Notifications
You must be signed in to change notification settings - Fork 0
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
Update tool to upload multiple files #20
Conversation
dist/index.js
Outdated
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.
just for testing purposes
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.
Thanks for your patience on this review 😅. I've been holding off since we can't use this until the changes on the platform side have been deployed.
If I'm understanding this change correctly, it looks like we're not actually capturing paginated results for any of the supported tools yet? That's okay but we should work to add support for Sonar issues and hotspots next.
@carlosu7 does it make sense to ship this change without actually implementing pagination for anything? Or should we just go ahead and do this when we implement Sonar pagination? Does the platform change remain backwards-compatible with the |
@drdavella i opened this PR because of the renaming of the form data We can hold this PR until pagination is ready, what do you think? |
@carlosu7 yes if you want to keep it open and add pagination for Sonar when it's ready, please just request my review again. |
@drdavella it is ready to review, here you can see a test with the following values: https://github.com/carlosu7/WebGoat_12_23/actions/runs/9670042632/job/26677944207 |
results: any | ||
} | ||
|
||
const MAX_PAGE_SIZE = 500; |
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.
@drdavella if you would like to test it, you will need to update this value and upload the dist/index.js 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.
@carlosu7 I think this looks good overall but I have just a few minor comments 🙏
Rename
file
property tofiles
and update endpoint to upload multiple files according to pixee platform changes