fix: deprecate --*.disabled CLI flags, wire up --mcp.disabled#3482
Open
souvikghosh04 wants to merge 3 commits intomainfrom
Open
fix: deprecate --*.disabled CLI flags, wire up --mcp.disabled#3482souvikghosh04 wants to merge 3 commits intomainfrom
souvikghosh04 wants to merge 3 commits intomainfrom
Conversation
- Hide --rest.disabled, --graphql.disabled, --mcp.disabled from --help (Hidden=true) - Add [Deprecated] prefix to HelpText for all three flags - Add missing deprecation warning for --mcp.disabled - Wire --mcp.disabled through TryDetermineIfApiIsEnabled (was previously a no-op) - Remove unused TryDetermineIfMcpIsEnabled helper - Use resolved enabled booleans for REST+GraphQL disabled-together check - Add 5 MCP test rows to TestEnabledDisabledFlagsForApis
souvikghosh04
requested review from
Aniruddh25,
JerryNixon,
RubenCerna2079,
aaronburtle,
anushakolan,
sourabh1007 and
vadeveka
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves dab init CLI UX by deprecating the --*.disabled flags (while keeping them functional), wiring up the previously no-op --mcp.disabled, and expanding tests to cover MCP enabled/disabled combinations.
Changes:
- Hide
--rest.disabled,--graphql.disabled,--mcp.disabledfrom help output and mark them as deprecated via HelpText + warnings. - Wire
--mcp.disabledthrough the shared enabled/disabled resolution logic and remove the MCP-specific helper. - Extend end-to-end CLI tests to validate MCP enabled/disabled flag behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Cli/ConfigGenerator.cs | Adds MCP deprecation warning, wires --mcp.disabled into the common resolver, and adjusts REST+GraphQL disabled validation logic. |
| src/Cli/Commands/InitOptions.cs | Hides deprecated --*.disabled flags from --help and prefixes HelpText with [Deprecated]. |
| src/Cli.Tests/EndToEndTests.cs | Adds MCP cases to enabled/disabled flag matrix and asserts MCP runtime config values. |
Comments suppressed due to low confidence (1)
src/Cli/ConfigGenerator.cs:250
- The init-time validation currently fails when both REST and GraphQL are disabled, even if MCP is enabled. This is stricter than runtime validation (RuntimeConfigValidator.ValidateGlobalEndpointRouteConfig only rejects the config when REST+GraphQL+MCP are all disabled), and it prevents generating an MCP-only config via
dab init --rest.enabled false --graphql.enabled false.
Consider updating this check to only error when all three endpoints are disabled (and update the error message accordingly).
if (!restEnabled && !graphQLEnabled)
{
_logger.LogError("Both Rest and GraphQL cannot be disabled together.");
return false;
}
| @@ -102,21 +102,28 @@ public static bool TryCreateRuntimeConfig(InitOptions options, FileSystemRuntime | |||
| // If --rest.disabled flag is included in the init command, we log a warning to not use this flag as it will be deprecated in future versions of DAB. | |||
Contributor
There was a problem hiding this comment.
nit: Change the comment so instead of saying the flag will be deprecated in the future, it will say that it is deprecated.
| " Use the --rest.enabled option instead."); | ||
| } | ||
|
|
||
| // If --graphql.disabled flag is included in the init command, we log a warning to not use this flag as it will be deprecated in future versions of DAB. |
Contributor
There was a problem hiding this comment.
nit: Change the comment so instead of saying the flag will be deprecated in the future, it will say that it is deprecated.
| " Use the --graphql.enabled option instead."); | ||
| } | ||
|
|
||
| // If --mcp.disabled flag is included in the init command, we log a warning to not use this flag as it will be deprecated in future versions of DAB. |
Contributor
There was a problem hiding this comment.
nit: Change the comment so instead of saying the flag will be deprecated in the future, it will say that it is deprecated.
| } | ||
|
|
||
| if (options.RestDisabled && options.GraphQLDisabled) | ||
| if (!restEnabled && !graphQLEnabled) |
Contributor
There was a problem hiding this comment.
Shouldn't this be changed so that all three rest, graphQL, and mcp cannot be disabled together?
anushakolan
reviewed
May 8, 2026
| } | ||
|
|
||
| if (options.RestDisabled && options.GraphQLDisabled) | ||
| if (!restEnabled && !graphQLEnabled) |
Contributor
There was a problem hiding this comment.
if (!restEnabled && !graphQLEnabled && !mcpEnabled)
{
_logger.LogError("At least one of REST, GraphQL, or MCP must be enabled.");
return false;
}
This might be a better way to update the code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why make this change?
disabled#3376dab inithas both--rest.disabledand--rest.enabled(same for graphql/mcp) for the same property. This is confusing UX. Additionally,--mcp.disabledwas defined but never wired up (no-op).What is this change?
--rest.disabled,--graphql.disabled,--mcp.disabledfrom--helpoutput (Hidden = true)[Deprecated]prefix to HelpText for all three flags--mcp.disabled(was missing — REST and GraphQL already had them)--mcp.disabledthroughTryDetermineIfApiIsEnabled(was previously a no-op)TryDetermineIfMcpIsEnabledhelperenabledbooleans instead of rawDisabledflagsBreaking change analysis
--*.disabledflags still work — they are just hidden from--help--mcp.disabledwas previously a no-op; now it actually disables MCPHow was this tested?
Sample Request(s)
NA