-
Notifications
You must be signed in to change notification settings - Fork 60
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
Quick Refactor: Moving DiagnosticSetting function to common place Cluster.ts. #1295
Conversation
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.
PR Overview
This PR refactors the location of the diagnosticSetting function by moving it to a central location in clusters.ts. The changes include relocating the function from periscopehelper.ts, updating imports in periscope.ts, and ensuring consistency in the diagnostic settings functionality.
Reviewed Changes
File | Description |
---|---|
src/commands/utils/clusters.ts | Added getClusterDiagnosticSettings to centralize diagnostic settings retrieval |
src/commands/periscope/helpers/periscopehelper.ts | Removed the duplicate diagnostic settings function |
src/commands/periscope/periscope.ts | Updated imports to reference the central implementation in clusters.ts |
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
be2866b
to
6808cd2
Compare
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.
PR Overview
This PR refactors the diagnostic settings function by moving its implementation from periscopehelper.ts to clusters.ts and updating corresponding imports.
- Moved the diagnosticSettings retrieval function to clusters.ts
- Removed duplicate handling from periscopehelper.ts
- Updated periscope.ts to import the refactored function
Reviewed Changes
File | Description |
---|---|
src/commands/utils/clusters.ts | Added getClusterDiagnosticSettings to retrieve diagnostic settings using getMonitorClient. |
src/commands/periscope/helpers/periscopehelper.ts | Removed the local implementation of getClusterDiagnosticSettings. |
src/commands/periscope/periscope.ts | Modified imports to use getClusterDiagnosticSettings from clusters.ts. |
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/commands/utils/clusters.ts:695
- Consider extracting a more descriptive error message (e.g., using e.message) to ensure clarity in the error reporting.
vscode.window.showErrorMessage(`Error fetching cluster diagnostic monitor: ${e}`);
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Tatsinnit <tatsatmishra@gmail.com>
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.
thanks for the PR, lgtm
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.
Refactor looks good
Hiya, This code move around is specifically for the
diagnosticSetting
function tocluster.ts
which will come handy towards the Retina hook work for theupload to blob scenario
The benefit is also that we should sit down to think if we want a testing framework for most of the
utils
function :) Jest or anything else?Gentle fyi and thanks to @tejhan, @ReinierCC , @bosesuneha , @davidgamero + @qpetraroia for reviews please ❤️
Thanks.