Skip to content

Conversation

@shivprakashmuley
Copy link
Owner

No description provided.

@coderabbitai
Copy link

coderabbitai bot commented Sep 29, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch collect-mg

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.


func initMustGatherPlan(o internalk8s.Openshift) []api.ServerTool {
return []api.ServerTool{{
Tool: api.Tool{

Choose a reason for hiding this comment

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

@shivprakashmuley while this is a good start - we might want to think about having more must gather tools tailored to generating a plan with different options specified through oc here. Some of the tools that come to mind:

  • must-gather-plan-with-since-time
  • must-gather-plan-with-image
  • must-gather-plan-with-all-images
  • must-gather-plan-with-namespace
  • must-gather-plan-with-host-network

},
},
Handler: func(params api.ToolHandlerParams) (*api.ToolCallResult, error) {
args := params.GetArguments()

Choose a reason for hiding this comment

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

as we write different tools with more options, it will increase complexity - so it would be good to have a utility file with functions specific to generating the pod spec

@Prashanth684
Copy link

while it is nice to provide a plan with a pod spec, I think we should also have tools provide the oc adm must-gather equivalent command in case users are just looking to collect through oc

suffix := rand.String(5)
namespaceName := fmt.Sprintf("openshift-must-gather-%s", suffix)
podName := fmt.Sprintf("must-gather-%s", suffix)
serviceAccountName := "must-gather-admin"

Choose a reason for hiding this comment

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

are we assuming that such an SA and namespace exist? can't we create an ephemeral namespace and use the default SA?

shivprakashmuley pushed a commit that referenced this pull request Oct 28, 2025
* feat: add cluster provider for kubeconfig

Signed-off-by: Calum Murray <[email protected]>

* feat: move server to use ClusterProvider interface

Signed-off-by: Calum Murray <[email protected]>

* feat: authentication middleware works with cluster provider

Signed-off-by: Calum Murray <[email protected]>

* fix: unit tests work after cluster provider changes

Signed-off-by: Calum Murray <[email protected]>

* feat: add tool mutator to add cluster parameter

Signed-off-by: Calum Murray <[email protected]>

* test: handle cluster parameter

Signed-off-by: Calum Murray <[email protected]>

* fix: handle lazy init correctly

Signed-off-by: Calum Murray <[email protected]>

* refactor: move to using multi-strategy ManagerProvider

Signed-off-by: Calum Murray <[email protected]>

* feat: add contexts_list tool

Signed-off-by: Calum Murray <[email protected]>

* refactor: make tool mutator generic between cluster/context naming

Signed-off-by: Calum Murray <[email protected]>

* feat: introduce tool filter

Signed-off-by: Calum Murray <[email protected]>

* refactor: use new ManagerProvider/mutator/filter within mcp server

Signed-off-by: Calum Murray <[email protected]>

* fix(test): tests expect context parameter in tool defs

Signed-off-by: Calum Murray <[email protected]>

* feat: auth handles multi-cluster case correctly

Signed-off-by: Calum Murray <[email protected]>

* fix: small changes from local testing

Signed-off-by: Calum Murray <[email protected]>

* chore: fix enum test

Signed-off-by: Calum Murray <[email protected]>

* review: Multi Cluster support (#1)

* nit: rename contexts_list to configuration_contexts_list

Besides the conventional naming, it helps LLMs understand the context of the tool by providing a certain level of hierarchy.

Signed-off-by: Marc Nuri <[email protected]>

* fix(mcp): ToolMutator doesn't rely on magic strings

Signed-off-by: Marc Nuri <[email protected]>

* refactor(api): don't expose ManagerProvider to toolsets

Signed-off-by: Marc Nuri <[email protected]>

* test(mcp): configuration_contexts_list basic tests

Signed-off-by: Marc Nuri <[email protected]>

* test(toolsets): revert edge-case test

This test should not be touched.

Signed-off-by: Marc Nuri <[email protected]>

* test(toolsets): add specific metadata tests for multi-cluster

Signed-off-by: Marc Nuri <[email protected]>

* fix(mcp): ToolFilter doesn't rely on magic strings (partially)

Signed-off-by: Marc Nuri <[email protected]>

* test(api): IsClusterAware and IsTargetListProvider default values

Signed-off-by: Marc Nuri <[email protected]>

* test(mcp): revert unneeded changes in mcp_tools_test.go

Signed-off-by: Marc Nuri <[email protected]>

---------

Signed-off-by: Marc Nuri <[email protected]>

* fix: always include configuration_contexts_list if contexts > 1

Signed-off-by: Calum Murray <[email protected]>

* feat: include server urls in configuration_contexts_list

Signed-off-by: Calum Murray <[email protected]>

---------

Signed-off-by: Calum Murray <[email protected]>
Signed-off-by: Marc Nuri <[email protected]>
Co-authored-by: Marc Nuri <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants