-
Notifications
You must be signed in to change notification settings - Fork 1.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
chore: move away from parallel usages of licenses v2 and v3 #6527
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
2 similar comments
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
👍 Looks good to me! Reviewed everything up to db0fefa in 1 minute and 18 seconds
More details
- Looked at
170
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. ee/query-service/license/db.go:114
- Draft comment:
Thectx
parameter is not used inGetActiveLicense
. Consider passing it to database operations for better context management. - Reason this comment was not posted:
Confidence changes required:50%
The functionGetActiveLicense
indb.go
is not using thecontext
parameter effectively. The context should be passed to any database operations to allow for better control over request lifecycles.
2. ee/query-service/license/db.go:137
- Draft comment:
Thectx
parameter is not used inGetActiveLicenseV3
. Consider passing it to database operations for better context management. - Reason this comment was not posted:
Confidence changes required:50%
The functionGetActiveLicenseV3
indb.go
is not using thecontext
parameter effectively. The context should be passed to any database operations to allow for better control over request lifecycles.
3. ee/query-service/license/db.go:83
- Draft comment:
Thectx
parameter is not used inGetActiveLicenseV2
. Consider passing it to database operations for better context management. - Reason this comment was not posted:
Confidence changes required:50%
The functionGetActiveLicenseV2
indb.go
is not using thecontext
parameter effectively. The context should be passed to any database operations to allow for better control over request lifecycles.
4. ee/query-service/app/api/license.go:96
- Draft comment:
Consider closing the request body after decoding to prevent resource leaks. Usedefer r.Body.Close()
after checking for errors. - Reason this comment was not posted:
Confidence changes required:50%
TheapplyLicense
function inlicense.go
usesjson.NewDecoder(r.Body).Decode(&l)
without closing the request body. This can lead to resource leaks. The request body should be closed after decoding.
5. ee/query-service/app/api/license.go:90
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets or styled components instead. This is also applicable at line 91. - Reason this comment was not posted:
Comment was on unchanged code.
6. ee/query-service/license/db.go:114
- Draft comment:
Do not add non-ClickHouse related functions to the ClickHouseReader interface. Use the DAO in the telemetry instance to access such information instead. This is also applicable at line 83. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Gd0ICVHWQZZqHnAj
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
Related Issues / PR's
contributes to - https://github.com/SigNoz/platform-pod/issues/304