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

feat: Support mcp setup on CLI #636

Merged
merged 3 commits into from
Jan 17, 2025
Merged

feat: Support mcp setup on CLI #636

merged 3 commits into from
Jan 17, 2025

Conversation

baxen
Copy link
Collaborator

@baxen baxen commented Jan 17, 2025

This includes a refactor of the config and configuration CLI to support the systems, and because they make config more complex we simplified other aspects like removing profiles. Instead hopefully the commands make it easy to change things on the fly.

There are now environment and CLI overrides for everything but systems. We can implement those in a followup pr (e.g. a list of which named systems we should run with)

Copy link

Desktop App for this PR

The following build is available for testing:

The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder.

This link is provided by nightly.link and will work even if you're not logged into GitHub.

Copy link
Collaborator

@kalvinnchau kalvinnchau left a comment

Choose a reason for hiding this comment

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

lgtm! going through the config workflow is nice

Comment on lines 250 to 260
let selected =
cliclack::multiselect("enable systems: (use \"space\" to toggle and \"enter\" to submit)")
.items(
&system_status
.iter()
.map(|(name, _)| (name, name.as_str(), ""))
.collect::<Vec<_>>(),
)
.initial_values(enabled_systems)
.interact()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't disable all my systems, if thats intentional we should add at least one must be enabled. "Input required" wasn't clear to me.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixing! just missed an option on the multiselect

Comment on lines 64 to 75
let config = Config::load().unwrap_or_else(|_| {
println!("No configuration found. Please run 'goose configure' first.");
process::exit(1);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to distinguish between "no config" vs "invalid" config?
I made my config file invalid yaml and just got

No configuration found. Please run 'goose configure' first.

)]
profile: Option<String>,
model: Option<String>,
Copy link
Collaborator

@salman1993 salman1993 Jan 17, 2025

Choose a reason for hiding this comment

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

should we remove the --provider and --model options? currently, if someone runs:

./target/debug/goose configure --provider openai --model gpt-4o

./target/debug/goose session
No configuration found. Please run 'goose configure' first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh good point. I do want to support these options for e.g. automations, but i need to remove the check for config if they are set

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will have to revisit this though in a followup PR where we introduce system management for automations, so right now the file is still required for systems

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds good

baxen added 3 commits January 17, 2025 13:39
This includes a refactor of the config and configuration CLI to support
the systems, and because they make config more complex we simplified
other aspects like removing profiles. Instead hopefully the commands
make it easy to change things on the fly.

There are now environment and CLI overrides for everything but systems.
We can implement those in a followup pr (e.g. a list of which named
systems we should run with)
@baxen baxen force-pushed the baxen/config-updates branch from 0afde15 to e90f66d Compare January 17, 2025 21:40
@baxen baxen merged commit 0d48d84 into v1.0 Jan 17, 2025
3 checks passed
acekyd pushed a commit that referenced this pull request Jan 21, 2025
@yingjiehe-xyz yingjiehe-xyz deleted the baxen/config-updates branch February 5, 2025 21:06
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