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.

Sign up for GitHub

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

Jump to bottom

Modify the cluster_id type usize to Option<usize> #385

Merged
merged 1 commit into from
Jan 13, 2025
Merged

Modify the cluster_id type usize to Option<usize> #385

merged 1 commit into from
Jan 13, 2025

Conversation

syncpark
Copy link
Contributor

@syncpark syncpark commented Jan 9, 2025

Close #384

Modify the type of cluster_id field of the detection event structures from usize to Option<usize>:

  • HttpThreat, ExtraThreat, NetworkThreat, WindowsThreat

@codecov Codecov
Copy link

codecov bot commented Jan 9, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 71.30%. Comparing base (3aebb1b) to head (7698364).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/graphql/event/http.rs 0.00% 1 Missing ⚠️
src/graphql/event/log.rs 0.00% 1 Missing ⚠️
src/graphql/event/network.rs 0.00% 1 Missing ⚠️
src/graphql/event/sysmon.rs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #385      +/-   ##
==========================================
- Coverage   71.32%   71.30%   -0.03%     
==========================================
  Files          67       67              
  Lines       14666    14666              
==========================================
- Hits        10461    10457       -4     
- Misses       4205     4209       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@syncpark syncpark force-pushed the modify-cluster-id-type branch from 16ebe25 to e43e55c Compare January 9, 2025 07:07
@syncpark
Copy link
Contributor Author

syncpark commented Jan 9, 2025 •
edited
Loading

depend on: petabi/review-database@3f99e80

@syncpark syncpark force-pushed the modify-cluster-id-type branch 4 times, most recently from 2edc767 to 85bd19e Compare January 10, 2025 01:01
@syncpark syncpark requested a review from msk January 10, 2025 01:11
msk
msk approved these changes Jan 11, 2025
View reviewed changes
Copy link
Contributor

@msk msk left a comment

Choose a reason for hiding this comment

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

Looks good to me. One minor suggestion: it might be worth mentioning the GraphQL schema change in the CHANGELOG, specifically that the type of WindowsThreat.clusterId has changed from Int to ID. However, I'll leave that up to your discretion as it's dependent on @aicers' policy. Otherwise, I'm good with this PR.

@msk msk mentioned this pull request Jan 12, 2025
Merged
@sehkone
Copy link
Contributor

sehkone commented Jan 12, 2025 •
edited
Loading

Looks good to me. One minor suggestion: it might be worth mentioning the GraphQL schema change in the CHANGELOG, specifically that the type of WindowsThreat.clusterId has changed from Int to ID. However, I'll leave that up to your discretion as it's dependent on @aicers' policy. Otherwise, I'm good with this PR.

@aicers' policy follows exactly what you pointed out.

@syncpark syncpark force-pushed the modify-cluster-id-type branch from 85bd19e to 733a9b9 Compare January 13, 2025 02:31
@syncpark
Copy link
Contributor Author

I've change the CHANGELOG.md

sehkone
sehkone reviewed Jan 13, 2025
View reviewed changes
CHANGELOG.md Outdated
- Modified the type of `cluster_id` field of the detection event structures from
`usize` to `Option<usize>`: `HttpThreat`, `ExtraThreat`, `NetworkThreat`,
`WindowsThreat`.
The GraphQL API for `WindowsThreat` event structure is changed to return `ID`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make this start with another bullet?

@syncpark syncpark force-pushed the modify-cluster-id-type branch from 733a9b9 to 7698364 Compare January 13, 2025 05:18
@sehkone sehkone merged commit a237786 into main Jan 13, 2025
8 of 10 checks passed
@sehkone sehkone deleted the modify-cluster-id-type branch January 13, 2025 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sehkone sehkone sehkone approved these changes

@msk msk msk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Modify the cluster_id type usize to Option<usize>
3 participants
@syncpark @sehkone @msk