Fix: Validate subscription before execution to prevent crash on missing credentials#275
Fix: Validate subscription before execution to prevent crash on missing credentials#275
Conversation
…sage Co-authored-by: mivano <[email protected]>
Co-authored-by: mivano <[email protected]>
Co-authored-by: mivano <[email protected]>
|
@copilot resolve conflicts as main now contains newer changes. So merge and resolve |
Co-authored-by: mivano <[email protected]>
|
@copilot are you sure all conflicts are resolved? GitHub PR is stating this: This branch has conflicts that must be resolved |
Resolved conflicts in AccumulatedCostCommand.cs and AdditionalCommandTests.cs by combining subscription validation with automatic timeframe detection.
…/mivano/azure-cost-cli into copilot/fix-azure-cost-cli-issue
Co-authored-by: mivano <[email protected]>
Fixed. The conflicts are now properly resolved. The issue was that my first merge attempt created a regular commit instead of a proper two-parent merge commit. I've now completed a proper git merge (commit d06cec9) that combines the subscription validation fix with the automatic timeframe detection from main, followed by a reconciliation merge (commit 2947988) to sync with the remote branch. All 141 tests pass. GitHub should now recognize the branch as conflict-free. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against b21ae3d |
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash that occurred when running azure-cost without arguments by adding validation logic to check for subscription availability before command execution. The fix moves subscription retrieval from the Execute phase to the Validate phase to fail gracefully with a helpful error message instead of crashing.
Key Changes:
- Added subscription validation in the Validate method to check if a subscription is available before execution
- Moved Azure CLI subscription retrieval from ExecuteAsync to Validate method
- Added comprehensive test coverage for subscription validation scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/Commands/AccumulatedCost/AccumulatedCostCommand.cs |
Added validation logic to check for subscription before execution and retrieve from Azure CLI if needed, moving this from ExecuteAsync to the Validate method |
tests/AzureCostCli.Tests/Commands/AdditionalCommandTests.cs |
Added three new test cases for subscription validation scenarios and updated existing tests to include subscription IDs to satisfy new validation requirements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Try to get subscription from Azure CLI | ||
| try | ||
| { | ||
| var subscriptionId = Guid.Parse(AzCommand.GetDefaultAzureSubscriptionId()); | ||
| settings.Subscription = subscriptionId; |
There was a problem hiding this comment.
The Validate method modifies the settings object by setting the Subscription property. This violates the principle that validation should not have side effects. If validation fails later or if the command is not executed, the settings object is left in a modified state. Consider moving the subscription retrieval logic to the beginning of ExecuteAsync, or return the retrieved subscription ID as part of a validation context without modifying the settings.
| // Try to get subscription from Azure CLI | |
| try | |
| { | |
| var subscriptionId = Guid.Parse(AzCommand.GetDefaultAzureSubscriptionId()); | |
| settings.Subscription = subscriptionId; | |
| // Try to get subscription from Azure CLI and ensure it is valid | |
| try | |
| { | |
| _ = Guid.Parse(AzCommand.GetDefaultAzureSubscriptionId()); |
| var subscriptionId = Guid.Parse(AzCommand.GetDefaultAzureSubscriptionId()); | ||
| settings.Subscription = subscriptionId; | ||
| } | ||
| catch (Exception) | ||
| { |
There was a problem hiding this comment.
The previous implementation included debug logging when retrieving the subscription ID from Azure CLI. This helpful debugging information has been removed in the refactoring. Consider adding similar debug logging when settings.Debug is true to help users troubleshoot subscription retrieval issues.
| var subscriptionId = Guid.Parse(AzCommand.GetDefaultAzureSubscriptionId()); | |
| settings.Subscription = subscriptionId; | |
| } | |
| catch (Exception) | |
| { | |
| if (settings.Debug) | |
| { | |
| AnsiConsole.MarkupLine("[grey]Attempting to retrieve default Azure subscription ID from Azure CLI...[/]"); | |
| } | |
| var subscriptionId = Guid.Parse(AzCommand.GetDefaultAzureSubscriptionId()); | |
| if (settings.Debug) | |
| { | |
| AnsiConsole.MarkupLine($"[grey]Retrieved subscription ID '{subscriptionId}' from Azure CLI.[/]"); | |
| } | |
| settings.Subscription = subscriptionId; | |
| } | |
| catch (Exception ex) | |
| { | |
| if (settings.Debug) | |
| { | |
| AnsiConsole.MarkupLine("[grey]Failed to retrieve subscription ID from Azure CLI.[/]"); | |
| AnsiConsole.WriteException(ex); | |
| } |
| [Fact] | ||
| public void Validate_WithNoSubscriptionAndNoAzureCLI_ReturnsError() | ||
| { | ||
| // Arrange | ||
| var settings = new AccumulatedCostSettings | ||
| { | ||
| Subscription = null // No subscription provided | ||
| }; | ||
| var context = CreateCommandContext(); | ||
|
|
||
| // Act | ||
| var result = _command.Validate(context, settings); | ||
|
|
||
| // Assert | ||
| result.Successful.ShouldBeFalse(); | ||
| result.Message.ShouldContain("No subscription ID provided"); | ||
| result.Message.ShouldContain("--help"); | ||
| } |
There was a problem hiding this comment.
The test Validate_WithNoSubscriptionAndNoAzureCLI_ReturnsError depends on the runtime environment (whether Azure CLI is installed and accessible). This makes the test non-deterministic and could fail in CI/CD environments where Azure CLI is installed. Consider refactoring AzCommand.GetDefaultAzureSubscriptionId to use dependency injection or making it mockable to ensure tests are isolated from the environment.
| var subscriptionId = Guid.Parse(AzCommand.GetDefaultAzureSubscriptionId()); | ||
| settings.Subscription = subscriptionId; | ||
| } | ||
| catch (Exception) |
There was a problem hiding this comment.
Catching all exceptions with a generic Exception catch block discards valuable diagnostic information. This makes it difficult to distinguish between different failure scenarios (e.g., Azure CLI not installed vs. not logged in vs. parse errors). Consider catching specific exceptions and providing more targeted error messages, or at least logging the exception details when Debug mode is enabled.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Fix: Running azure-cost without arguments shows helpful error instead of crashing ✅
Summary of Changes
Problem
When running
azure-costwithout any arguments, the application crashed with an unhandled exception when trying to retrieve the subscription ID from Azure CLI.Solution
Successfully merged main branch changes with proper conflict resolution:
Merge Commits:
Resolved Conflicts:
AccumulatedCostCommand.cs: Combined subscription validation with newApplyAutoTimeframe()call andGetFromDate()/GetToDate()methodsAdditionalCommandTests.cs: Added subscription IDs to all new tests from main branchTesting
✅ All 10 AccumulatedCostCommandTests pass
✅ All 141 total tests pass
✅ Build succeeds with no errors
✅ Conflicts fully resolved
GitHub should now recognize the branch as properly merged with main.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.