-
Notifications
You must be signed in to change notification settings - Fork 5
First cut of the smithy-mcp CLI #677
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
base: main
Are you sure you want to change the base?
Conversation
ebda0cc
to
5dd3763
Compare
...s-codegen/src/main/java/software/amazon/smithy/java/codegen/types/JavaTypeCodegenPlugin.java
Outdated
Show resolved
Hide resolved
.../src/main/java/software/amazon/smithy/java/aws/mcp/cli/commands/AddAwsServiceToolBundle.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/software/amazon/smithy/java/aws/servicebundle/bundler/AwsServiceBundler.java
Outdated
Show resolved
Hide resolved
private static String loadModel(String path) { | ||
try (var reader = new BufferedReader(new InputStreamReader( | ||
Objects.requireNonNull(AwsServiceBundler.class.getResourceAsStream(path)), | ||
StandardCharsets.UTF_8))) { | ||
return reader.lines().collect(Collectors.joining()); | ||
return reader.lines().collect(Collectors.joining("\n")); | ||
} catch (Exception e) { | ||
throw new RuntimeException(e); | ||
} |
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.
We should hoist this to Bundler as a protected final method. I've already messed this up twice and I'll keep messing it up every time I rewrite it.
mcp/mcp-cli-api/src/main/java/software/amazon/smithy/java/mcp/cli/ConfigUtils.java
Outdated
Show resolved
Hide resolved
mcp/mcp-cli-api/src/main/java/software/amazon/smithy/java/mcp/cli/ConfigUtils.java
Outdated
Show resolved
Hide resolved
mcp/mcp-cli-api/src/main/java/software/amazon/smithy/java/mcp/cli/ConfigUtils.java
Outdated
Show resolved
Hide resolved
execute(config); | ||
return 0; | ||
} catch (IllegalArgumentException e) { | ||
System.out.println("Invalid input : [" + e.getMessage() + "]"); |
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.
- Why println here and LOG in the Exception arm?
- Please don't discard the stack trace. Do
e.printStackTrace(System.out)
if you must, although we should likely be using the logger here (unless you intended for this to go to the mcp client's logs, in which case stderr is a better choice)
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.
So the idea was we only emit messages for errors and only print stack traces. I didn't want to log a stack trace for wrong inputs.
I think I need to do a second pass on logging, and also configure the logger. Will cleanup logging and println in the CLIs in a followup.
|
||
@Override | ||
public String[] getVersion() throws Exception { | ||
return new String[] {IoUtils.readUtf8Resource(VersionProvider.class, "VERSION").trim()}; |
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.
formatting
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.
I tried adding a space but Spotless keeps changing it back to this.
mcp/mcp-cli/src/main/java/software/amazon/smithy/java/mcp/cli/commands/AddSmithyToolBundle.java
Outdated
Show resolved
Hide resolved
|
||
extra["displayName"] = "Smithy :: Java :: Model Bundler" | ||
extra["moduleName"] = "software.amazon.smithy.java.modelbundle.api" | ||
extra["displayName"] = "Smithy :: Java :: MCP Bundler" |
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.
Leave this in the current module. There's nothing intrinsically tied to MCP in this module, and I want to use the bundle format to drive smithy-call
in the future.
var endpoint = URI.create(Objects.requireNonNull(serviceMetadata.getEndpoints().get(input.getAwsRegion()), | ||
"no endpoint for region " + input.getAwsRegion())); | ||
|
||
try (var sdkCredentialsProvider = ProfileCredentialsProvider.create(input.getAwsProfileName())) { |
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.
sdkCredentialsProvider
will be closed when we return from this method but the reference will live in identityResolver
. I don't think we want the try-with-resources here.
|
||
public abstract Bundle bundle(); | ||
|
||
protected String loadModel(String path) { |
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.
This can be static or final
var modelAssemble = new ModelAssembler().putProperty(ModelAssembler.ALLOW_UNKNOWN_TRAITS, true) | ||
.addUnparsedModel("bundle.json", bundle.getModel()); | ||
var additionalInput = bundle.getAdditionalInput(); | ||
if (additionalInput != null) { |
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.
We're only running the streaming operation redaction logic when there's additional inputs, but we should always be doing it
skipOperation = true; | ||
break; |
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.
Did checkstyle complain about using a labeled break or is this a personal style choice?
|
||
if (!CONFIG_PATH.toFile().exists()) { | ||
// Create an empty JSON object as the default config | ||
Files.write(CONFIG_PATH, toJson(DEFAULT_CONFIG_PROVIDER.getConfig()), StandardOpenOption.CREATE_NEW); |
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.
This will throw if the file was created in between us check if it existed and us trying to create it
* @throws IOException If there's an error writing to the file | ||
*/ | ||
public static void updateConfig(Config config) throws IOException { | ||
Files.write(CONFIG_PATH, toJson(config), StandardOpenOption.CREATE_NEW); |
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.
CREATE_NEW causes this to throw if the file already exists, so this will (almost?) always fail
|
||
import software.amazon.smithy.java.mcp.cli.model.Config; | ||
|
||
public class EmptyDefaultConfigProvider implements DefaultConfigProvider { |
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.
final
.putSupportedAuthSchemes(StaticAuthSchemeResolver.staticScheme(authScheme)); | ||
public <C extends Client, B extends Client.Builder<C, B>> B configureClient(B clientBuilder) { | ||
clientBuilder.addInterceptor(new AwsServiceClientInterceptor(serviceMetadata, authScheme)); | ||
clientBuilder.endpointResolver(EndpointResolver.staticEndpoint("http://dummyurl.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.
use localhost. let's avoid using anything that could result in traffic being sent to a domain squatter.
Issue #, if available:
Description of changes:
Currently supports two commands,
configure
-> This has further subcommands which allow adding bundles to the config.start-server
-> This starts an MCP server for a given pre-configured bundle.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.