Skip to content

Remove all mention of unreleased service#17

Merged
kyoncal merged 3 commits into
aws:mainfrom
kyoncal:docs/remove-eks-mcp
Oct 1, 2025
Merged

Remove all mention of unreleased service#17
kyoncal merged 3 commits into
aws:mainfrom
kyoncal:docs/remove-eks-mcp

Conversation

@kyoncal

@kyoncal kyoncal commented Sep 30, 2025

Copy link
Copy Markdown
Contributor
  • Remove every mention of unreleased service in docs and code

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@kyoncal kyoncal requested review from acmlau and arangatang and removed request for arangatang September 30, 2025 12:40
Comment thread aws_mcp_proxy/sigv4_helper.py Outdated

def create_sigv4_client(
service: str = 'eks-mcp',
service: str = 'your-service',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we should do a default service string here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There needs to be some string as the default service. I suggest the empty string.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We have https://github.com/aws/aws-mcp-proxy/blob/main/aws_mcp_proxy/utils.py#L54-L83, which ensures we find the correct service string, why can't we leave it without a default? 🤔

@kyoncal kyoncal Sep 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The service string will indeed be found and replaced after the fact if none is given, however, given that it is a positional argument, if it's not given a default value, one must be provided. Otherwise uses like this won't type check.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rather set it to None then empty string

@kyoncal kyoncal force-pushed the docs/remove-eks-mcp branch from 21a8039 to 328a837 Compare September 30, 2025 13:50
Comment thread README.md Outdated
Comment thread aws_mcp_proxy/sigv4_helper.py Outdated

def create_sigv4_client(
service: str = 'eks-mcp',
service: str = '',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we make this required, without default parameter? using "" as the signature code will not work. This will help preventing contributors making mistake

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the best solution is to make it optional, since it gets inferred if not explicitly provided by the user anyways. In the latest revision, I've made it more elegant by making the string optional as such.
service: Optional[str] = None,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@wzxxing upon further investigation, I believe your solution may be the best. Changing the type from str to Optional[str] is not possible without major refactor. I agree that making it a required argument reduces confusion (albeit a little less elegant).

cc: @acmlau

Comment thread aws_mcp_proxy/server.py
@kyoncal kyoncal requested review from acmlau and wzxxing September 30, 2025 14:15
@kyoncal kyoncal force-pushed the docs/remove-eks-mcp branch from 328a837 to cb99eb9 Compare September 30, 2025 14:16
@kyoncal kyoncal requested a review from arangatang October 1, 2025 11:59
Kyon Caldera added 3 commits October 1, 2025 14:10
@kyoncal kyoncal force-pushed the docs/remove-eks-mcp branch from 5b3b2b0 to d1fafd9 Compare October 1, 2025 12:11
@kyoncal kyoncal self-assigned this Oct 1, 2025
@kyoncal kyoncal merged commit dff47c8 into aws:main Oct 1, 2025
3 checks passed
@kyoncal kyoncal changed the title Remove all mention of eks-mcp Remove all mention of unreleased service Oct 2, 2025
@aws aws deleted a comment from arangatang Oct 2, 2025
@aws aws deleted a comment from arangatang Oct 2, 2025
@kyoncal kyoncal deleted the docs/remove-eks-mcp branch October 2, 2025 09:16
@aws aws deleted a comment from acmlau Oct 2, 2025
@aws aws deleted a comment from wzxxing Oct 2, 2025
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.

4 participants