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

fix(api): add newResponse for function SearchAllPages #770

Merged
merged 5 commits into from
May 5, 2022

Conversation

aircraft-cerier
Copy link
Contributor

@aircraft-cerier aircraft-cerier commented Apr 14, 2022

This is necessary as using the current response to iterate over pages
corrupts data that are slices, such as the tags within imageInfo. This is because
the memory address from the previous response will still contain data from the previous request.
By creating a new response using the nextPage url we get a new spot in memory to put the next
requests data.

@afiune afiune requested a review from a team April 14, 2022 16:49
@afiune afiune added the bug Something isn't working label Apr 14, 2022
@afiune afiune changed the title add newResponse for function SearchAllPages fix(api): add newResponse for function SearchAllPages Apr 14, 2022
@afiune
Copy link
Contributor

afiune commented Apr 15, 2022

Make it so!

@afiune
Copy link
Contributor

afiune commented Apr 19, 2022

@aircraft-cerier Thank you so much for your first contribution to this project! 🎉 I think this update makes sense and it will help us fix the bug you are experiencing, but for us to merge this change we need two things:

  1. We require all commits to be signed with a GPG key https://github.com/lacework/go-sdk/blob/main/DEVELOPER_GUIDELINES.md#signed-commits
  2. We had some pipeline issues that we have fixed in the main branch so please, rebase your PR so that the pipeline goes green

Please let us know if you need any help with these two requests. We look forward to merging this pull request! 💯

@aircraft-cerier
Copy link
Contributor Author

@afiune I think i fixed your 2 requests, let me know if i'm missing something

@rmoles
Copy link
Collaborator

rmoles commented May 3, 2022

Make it so!

Copy link
Collaborator

@rmoles rmoles left a comment

Choose a reason for hiding this comment

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

image
Thanks for the contribution @aircraft-cerier!

@dmurray-lacework
Copy link
Collaborator

Hi @aircraft-cerier I see one of your commits is still missing the "verified" icon. Can you do a rebase, then push with a signed commit.

@rmoles
Copy link
Collaborator

rmoles commented May 5, 2022

Make it so!

This is necessary as using the current response to iterate over pages
corrupts data that are slices, such as the tags within imageInfo. This is beacause
the memory address from the previous response will still contain data from the previous request.
By creating a new response using the nextPage url we get a new spot in memory to put the next
requests data.

Signed-off-by: aircraft-cerier <[email protected]>
This is necessary as using the current response to iterate over pages
corrupts data that are slices, such as the tags within imageInfo. This is beacause
the memory address from the previous response will still contain data from the previous request.
By creating a new response using the nextPage url we get a new spot in memory to put the next
requests data.

Signed-off-by: aircraft-cerier <[email protected]>
Signed-off-by: aircraft-cerier <[email protected]>
Signed-off-by: aircraft-cerier <[email protected]>
Signed-off-by: aircraft-cerier <[email protected]>
@rmoles
Copy link
Collaborator

rmoles commented May 5, 2022

Make it so!

@rmoles rmoles merged commit 474a163 into lacework:main May 5, 2022
@lacework-releng lacework-releng mentioned this pull request May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants