-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Fleet] Fixes wrong artifact Id on bulkCreate response #160710
[Fleet] Fixes wrong artifact Id on bulkCreate response #160710
Conversation
Pinging @elastic/security-defend-workflows (Team:Defend Workflows) |
Pinging @elastic/fleet (Team:Fleet) |
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 am not familiar with this, but from looking at code I have to ask a few questions, thanks!
@@ -186,13 +186,25 @@ describe('When using the artifacts services', () => { | |||
}); | |||
|
|||
it('should create and return a multiple big artifacts', async () => { | |||
const { ...generatedArtifact1 } = generateArtifactMock({ encodedSize: 5_000_500 }); | |||
const { ...generatedArtifact1 } = generateArtifactMock({ |
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.
why do you need to spread rest like this?
const { ...generatedArtifact1 } = generateArtifactMock({ | |
const generatedArtifact1 = generateArtifactMock({ |
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.
Good point, this code was already there, but what you pointed out makes totally sense
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.
const { ...generatedArtifact1 } = generateArtifactMock({ | ||
encodedSize: 5_000_500, | ||
decodedSha256: '1234', | ||
}); | ||
const newBigArtifact1 = generatedArtifact1; |
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.
sorry for the rookie question, but what is the reason to save the reference to a new const?
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.
Also changed in the commit above, it has done on this way before, I guess because it was taking more props
From a conversation above with @gergoabraham, the non sorted array of artifacts for the response has been moved out of the logic that generates the artifact batches. Commit: 5e455b9 cc: @ashokaditya @juliaElastic in case you want to review again, the logic has changed but the input/output is still the same. Thanks @gergoabraham for pointing this out!! Update: Remove the empty array initialization: 1688315 |
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.
thank you for the modifications, @dasansol92! 🙌
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.
LGTM, thanks for the changes 👍
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
To update your PR or re-run it, just comment with: |
## Summary The artifact Id on the bulkCreateArtifact response was wrong. There is no issue currently as this id is not used, but it could end up in a bug if at some point someone somewhere uses this wrong id. Improved unit test to handle this. Related PR: elastic#159187 (cherry picked from commit 4cc04bd)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
#160752) # Backport This will backport the following commits from `main` to `8.9`: - [[Fleet] Fixes wrong artifact Id on bulkCreate response (#160710)](#160710) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"David Sánchez","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-06-28T13:49:31Z","message":"[Fleet] Fixes wrong artifact Id on bulkCreate response (#160710)\n\n## Summary\r\n\r\nThe artifact Id on the bulkCreateArtifact response was wrong. There is\r\nno issue currently as this id is not used, but it could end up in a bug\r\nif at some point someone somewhere uses this wrong id.\r\n\r\nImproved unit test to handle this.\r\n\r\nRelated PR: https://github.com/elastic/kibana/pull/159187","sha":"4cc04bd8e35320603a592de619791f26ce3b74a8","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Fleet","Team:Defend Workflows","v8.9.0","v8.10.0"],"number":160710,"url":"https://github.com/elastic/kibana/pull/160710","mergeCommit":{"message":"[Fleet] Fixes wrong artifact Id on bulkCreate response (#160710)\n\n## Summary\r\n\r\nThe artifact Id on the bulkCreateArtifact response was wrong. There is\r\nno issue currently as this id is not used, but it could end up in a bug\r\nif at some point someone somewhere uses this wrong id.\r\n\r\nImproved unit test to handle this.\r\n\r\nRelated PR: https://github.com/elastic/kibana/pull/159187","sha":"4cc04bd8e35320603a592de619791f26ce3b74a8"}},"sourceBranch":"main","suggestedTargetBranches":["8.9"],"targetPullRequestStates":[{"branch":"8.9","label":"v8.9.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/160710","number":160710,"mergeCommit":{"message":"[Fleet] Fixes wrong artifact Id on bulkCreate response (#160710)\n\n## Summary\r\n\r\nThe artifact Id on the bulkCreateArtifact response was wrong. There is\r\nno issue currently as this id is not used, but it could end up in a bug\r\nif at some point someone somewhere uses this wrong id.\r\n\r\nImproved unit test to handle this.\r\n\r\nRelated PR: https://github.com/elastic/kibana/pull/159187","sha":"4cc04bd8e35320603a592de619791f26ce3b74a8"}}]}] BACKPORT--> Co-authored-by: David Sánchez <[email protected]>
Summary
The artifact Id on the bulkCreateArtifact response was wrong. There is no issue currently as this id is not used, but it could end up in a bug if at some point someone somewhere uses this wrong id.
Improved unit test to handle this.
Related PR: #159187