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

chore: add pvcs list #6654

Merged
merged 3 commits into from
Dec 19, 2024
Merged

chore: add pvcs list #6654

merged 3 commits into from
Dec 19, 2024

Conversation

srikanthccv
Copy link
Member

@srikanthccv srikanthccv commented Dec 17, 2024

Summary

Part of #5373

Similar to what we did for other components like pods and nodes but for pvcs.


Important

Add functionality to list Persistent Volume Claims (PVCs) with new routes, handlers, repository, and models.

  • Behavior:
    • Add PVCs listing functionality similar to pods and nodes.
    • New routes for PVCs in http_handler.go with handlers getPvcList, getPvcAttributeKeys, and getPvcAttributeValues.
  • Repositories:
    • Add PvcsRepo in pvcs.go for handling PVC data.
    • Implement methods GetPvcList, GetPvcAttributeKeys, and GetPvcAttributeValues in PvcsRepo.
  • Queries:
    • Define PvcsTableListQuery in pvcs_query.go for querying PVC data.
    • Use getParamsForTopVolumes in common.go for PVC query parameters.
  • Models:
    • Add VolumeListRequest and VolumeListResponse in infra.go for PVC requests and responses.

This description was created by Ellipsis for 20a7c49. It will automatically update as commits are pushed.

@github-actions github-actions bot added the chore label Dec 17, 2024
@srikanthccv srikanthccv changed the base branch from develop to main December 19, 2024 10:33
@srikanthccv srikanthccv marked this pull request as ready for review December 19, 2024 10:35
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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! Reviewed everything up to 20a7c49 in 1 minute and 54 seconds

More details
  • Looked at 751 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. pkg/query-service/app/inframetrics/pvcs.go:112
  • Draft comment:
    Modifying the GroupBy field of the request object directly can lead to unexpected behavior if the request object is reused. Consider creating a copy of the GroupBy field and modifying the copy instead.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  1. The method is internal and the request object's scope is limited to this function. 2. The modification is simple - just appending new items if they don't exist. 3. There's no evidence of the request being reused after this method. 4. Making a copy would add complexity without clear benefit. 5. The current approach is straightforward and contained.
    I could be wrong about request reuse - there might be concurrent access patterns I'm not seeing in this file.
    Even if there was concurrent access, that would be a higher-level architectural issue that should be handled by the caller, not this internal method.
    The comment should be deleted as it suggests adding complexity without clear benefit, given this is an internal method with contained scope.
2. pkg/query-service/app/http_handler.go:210
  • Draft comment:
    Avoid adding non-ClickHouse related functions to the ClickHouseReader interface. Use the DAO in the telemetry instance to access such information instead.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_M3Eq4WmsGOCl9cjq


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@srikanthccv srikanthccv enabled auto-merge (squash) December 19, 2024 11:59
@srikanthccv srikanthccv merged commit fa90fad into main Dec 19, 2024
16 of 17 checks passed
@srikanthccv srikanthccv deleted the pvcs branch December 19, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants