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

fix: analyze command default backend bug #966

Merged
merged 2 commits into from
Feb 28, 2024

Conversation

VaibhavMalik4187
Copy link
Contributor

Closes #902

📑 Description

Now, the default value of the backend flag for the analyze command will be an empty string. The NewAnalysis function has been modified to use the default backend set by the user if the backend flag is not provided and the defaultprovider is set in the config file. Otherwise, the backend will be set to "openai".

✅ Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

@VaibhavMalik4187
Copy link
Contributor Author

@JuHyung-Son could you please go through this PR whenever you have some time? Thanks!

@AlexsJones
Copy link
Member

This looks like it's just moving the functionality elsewhere?

Copy link
Contributor

@JuHyung-Son JuHyung-Son left a comment

Choose a reason for hiding this comment

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

thanks, cli backend options are working well. and i left 2 comments

@VaibhavMalik4187
Copy link
Contributor Author

VaibhavMalik4187 commented Feb 18, 2024

This looks like it's just moving the functionality elsewhere?

Consider the following scenario if this fix was not present:

  1. I set the default provider to say localai
  2. Suppose, I've also configured an "openai" backend and instead of using the default provider, I want to use openai to analyze the cluster.
  3. How would I do that by specifying the backend flag? These conditions would enter the following if block and force set the backend to localai:
if configAI.DefaultProvider != "" && backend == "openai" {

However, using the fix I proposed, the user can also specify openai as the backend for the analyze command. And setting the default value of the backend flag as "" helps in identifying if the user wanted to openai or not.

@JuHyung-Son
Copy link
Contributor

i think i looks good. and works well.
and im not sure this problem is related to this pr. doe k8sgpt serve works??

CleanShot 2024-02-19 at 11 10 47

@VaibhavMalik4187
Copy link
Contributor Author

i think i looks good. and works well. and im not sure this problem is related to this pr. doe k8sgpt serve works??

CleanShot 2024-02-19 at 11 10 47

I think you hunch is right, this is a separate problem because I saw changes related to cmd/serve/serve.go when rebasing #929 on main. I'll look into it and open a PR with the fix.

Copy link
Contributor

@JuHyung-Son JuHyung-Son left a comment

Choose a reason for hiding this comment

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

sorry for delay, this looks good to me.
and cli works well

Now, the default value of the `backend` flag for the analyze command
will be an empty string. And the `NewAnalysis` function has been
modified to use the default backend set by the user if the backend flag
is not provided and the `defaultprovider` is set in the config file.
Otherwise, backend will be set to "openai".

Fixes: k8sgpt-ai#902

Signed-off-by: VaibhavMalik4187 <vaibhavmalik2018@gmail.com>
@VaibhavMalik4187
Copy link
Contributor Author

VaibhavMalik4187 commented Feb 21, 2024

sorry for delay, this looks good to me. and cli works well

No worries, I've rebased the PR to include the latest changes on main.

Copy link
Contributor

@JuHyung-Son JuHyung-Son left a comment

Choose a reason for hiding this comment

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

thanks for fixing bug.

@AlexsJones AlexsJones merged commit aab8d77 into k8sgpt-ai:main Feb 28, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug]: analyze`s --backend arg error
3 participants