Fix case-insensitive Service Fabric durability parsing - #30015
Fix case-insensitive Service Fabric durability parsing#30015Aditya Pujara (a0x1ab) with Copilot wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
| private sealed class TestServiceFabricClusterCmdlet : ServiceFabricClusterCmdlet | ||
| { | ||
| public DurabilityLevel GetNodeTypeDurabilityLevel(string durabilityLevel) | ||
| { | ||
| return GetDurabilityLevel(durabilityLevel); | ||
| } |
Live test results — TestFx
|
![azure-client-tools-agent[bot]](https://avatars.githubusercontent.com/in/4205534?s=60&v=4){kind=small}
There was a problem hiding this comment.
Live-test review — build failure
The TestFx (Record) live test for the changed test file failed during the build step, before any test could run.
Test project: src/ServiceFabric/ServiceFabric.Test
Changed test file: src/ServiceFabric/ServiceFabric.Test/ScenarioTests/TestServiceFabric.cs
Filter: FullyQualifiedName~TestServiceFabric
Build errors (6 total):
src/ServiceFabric/ServiceFabric.Test/ScenarioTests/TestServiceFabric.cs(31,20): error CS0246: The type or namespace name 'DurabilityLevel' could not be found (are you missing a using directive or an assembly reference?)
src/ServiceFabric/ServiceFabric.Test/ScenarioTests/TestServiceFabric.cs(36,20): error CS0246: The type or namespace name 'DurabilityLevel' could not be found (are you missing a using directive or an assembly reference?)
src/ServiceFabric/ServiceFabric.Test/ScenarioTests/TestServiceFabric.cs(89,31): error CS0103: The name 'DurabilityLevel' does not exist in the current context
src/ServiceFabric/ServiceFabric.Test/ScenarioTests/TestServiceFabric.cs(90,31): error CS0103: The name 'DurabilityLevel' does not exist in the current context
src/ServiceFabric/ServiceFabric.Test/ScenarioTests/TestServiceFabric.cs(91,29): error CS0103: The name 'DurabilityLevel' does not exist in the current context
src/ServiceFabric/ServiceFabric.Test/ScenarioTests/TestServiceFabric.cs(92,86): error CS0246: The type or namespace name 'DurabilityLevel' could not be found (are you missing a using directive or an assembly reference?)
The test file references a DurabilityLevel type/enum that either doesn't exist in the referenced SDK/namespace or is missing a using directive. Since this PR's purpose is "Fix case-insensitive Service Fabric durability parsing," please:
- Confirm the correct fully-qualified type/enum for durability level values in the Service Fabric management SDK used by this module (it may need a
using Microsoft.Azure.Management.ServiceFabric.Models;or similar, or the enum name may have changed/been renamed). - Update
TestServiceFabric.cs(and any production code undersrc/ServiceFabric/ServiceFabric.PowerShell.Cmdletsit depends on) so the project builds cleanly. - Push the fix so CI/live-test can re-run.
Live-test run: https://github.com/Azure/issue-sentinel/actions/runs/31827071365
Regular CI checks on this PR are currently green, but the live test build failure must be resolved before this can be merged.
Posted by agent-assist (autonomous bug-fix pipeline).
azure-client-tools-agent
Bot
added
the
azure-client-tools-agent
|
Started a Copilot task using Posted by agent-assist (autonomous bug-fix pipeline). |
…Fabric.cs Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- src/ServiceFabric/ServiceFabric/Properties/Resources.Designer.cs: Generated file
Suppressed comments (2)
src/ServiceFabric/ServiceFabric/Commands/ServiceFabricClusterCmdlet.cs:134
Enum.TryParsewill returntruefor numeric strings (e.g., "1" or "999"), producing enum values that are not actually defined (or bypassing the intended "valid values" check). That means malformed stored durability properties may still slip through instead of throwing the new actionable error. Consider additionally validating that the parsed value is a defined enum name (and trimming input) before returning it.
protected DurabilityLevel GetDurabilityLevel(string durabilityLevel)
{
DurabilityLevel parsedDurabilityLevel;
if (Enum.TryParse(durabilityLevel, true, out parsedDurabilityLevel))
{
src/ServiceFabric/ServiceFabric.Test/ScenarioTests/TestServiceFabric.cs:110
- The negative-path test only exercises a non-numeric invalid value ("invalid"). Since
Enum.TryParsealso accepts numeric strings, add coverage for numeric/undefined inputs (e.g., "1" and "999") to ensure malformed stored durability values reliably trigger the actionable error instead of being interpreted as enum integers.
[Fact]
public void GetDurabilityLevelReportsInvalidValue()
{
var cmdlet = new TestServiceFabricClusterCmdlet();
Live test results — TestFx
|
There was a problem hiding this comment.
Automated Review — Live Test Failed
The dispatched live test (TestFx Record, PowerShell live-test workflow) for
this PR's changed test file failed on head SHA 9630dadcde8e8d522bc0a795efc7af0a7525dead.
Test project: src/ServiceFabric/ServiceFabric.Test
Filter: FullyQualifiedName~TestServiceFabric
Changed test file: src/ServiceFabric/ServiceFabric.Test/ScenarioTests/TestServiceFabric.cs
Result: ❌ FAIL (exit code 1)
Run: https://github.com/Azure/issue-sentinel/actions/runs/31831639663
Relevant error from the test output:
/home/runner/.nuget/packages/microsoft.identity.client.nativeinterop/0.20.4/lib/netstandard2.0/libmsalruntime: cannot open shared object file: No such file or directory
at Microsoft.Azure.Commands.ServiceFabric.Commands.ServiceFabricClusterCmdlet.GetCurrentCluster() in src/ServiceFabric/ServiceFabric/Commands/ServiceFabricClusterCmdlet.cs:line 88
at Microsoft.Azure.Commands.ServiceFabric.Commands.GetAzureRmServiceFabricCluster.ExecuteCmdlet() in src/ServiceFabric/ServiceFabric/Commands/GetAzureRmServiceFabricClusterResouce.cs:line 54
Script Stack Trace: at WaitForClusterReadyState, .../ScenarioTests/Common.ps1: line 181
at WaitForClusterReadyStateIfRecord, .../ScenarioTests/Common.ps1: line 170
at Test-AddAzureRmServiceFabricNodeType, .../ScenarioTests/TestServiceFabric.ps1: line 120
The failing scenario is Test-AddAzureRmServiceFabricNodeType, which calls
GetAzureRmServiceFabricCluster → ServiceFabricClusterCmdlet.GetCurrentCluster().
The exception is cannot open shared object file for
libmsalruntime, which surfaces from MSAL's native interop layer during
authentication in the test cmdlet path — this may point at an MSAL/auth
dependency resolution issue triggered by this PR's change, or at a native
library not being restored/copied to the test output directory. Please
investigate why libmsalruntime isn't being found from the test bin output at
runtime for this scenario, verify the fix for AzServiceFabricDurability case
sensitivity doesn't affect MSAL/auth dependency loading, and push a fix (or
confirm this is a pre-existing environment issue and adjust/re-run
accordingly). CI checks on the PR itself are all green; only this live test
step is failing.
Posted by agent-assist (autonomous bug-fix pipeline).
|
Started a Copilot task using Posted by agent-assist (autonomous bug-fix pipeline). |
Live test results — TestFx
|
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- src/ServiceFabric/ServiceFabric/Properties/Resources.Designer.cs: Generated file
Suppressed comments (1)
src/ServiceFabric/ServiceFabric.Test/ScenarioTests/DurabilityLevelTests.cs:22
- CI runs are filtered to tests with the xUnit trait "AcceptanceType=CheckIn" (tools/ExecuteCIStep.ps1:333). Adding the trait at the class level ensures these regression tests actually run in CI.
namespace Microsoft.Azure.Commands.ServiceFabric.Test.ScenarioTests
{
public class DurabilityLevelTests
{
| using Microsoft.Azure.Commands.ServiceFabric.Commands; | ||
| using Microsoft.Azure.Commands.ServiceFabric.Models; | ||
| using Xunit; |
| var exception = Assert.Throws<System.Management.Automation.PSInvalidOperationException>( | ||
| () => cmdlet.GetNodeTypeDurabilityLevel("invalid")); | ||
|
|
||
| Assert.Contains("Valid values are Bronze, Silver, and Gold.", exception.Message); |
|
@azure-client-tools-agent please fix the ci error. |
|
Started a Copilot task using |
…sage Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- src/ServiceFabric/ServiceFabric/Properties/Resources.Designer.cs: Generated file
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/ServiceFabric/ServiceFabric/Commands/ServiceFabricClusterCmdlet.cs:136
GetDurabilityLevel(string)callsTrim()multiple times and formats the error message with the untrimmed input. This makes the method harder to read and can produce confusing errors for inputs that contain leading/trailing whitespace. Consider trimming once and reusing the trimmed value for both parsing and error reporting.
DurabilityLevel parsedDurabilityLevel;
if (!int.TryParse(durabilityLevel?.Trim(), out _) &&
Enum.TryParse(durabilityLevel?.Trim(), true, out parsedDurabilityLevel) &&
Enum.IsDefined(typeof(DurabilityLevel), parsedDurabilityLevel))
{
src/ServiceFabric/ServiceFabric/ChangeLog.md:23
- The second changelog entry describes behavior that is part of the same fix, but it doesn't include the issue reference. Adding the same issue link keeps the release notes consistent and makes it easier to trace back to the report.
* Fixed `Update-AzServiceFabricDurability` to accept durability values stored with lowercase casing.
- Fixed issue [#30014]
* Fixed `Update-AzServiceFabricDurability` and related cmdlets to reject numeric strings (e.g., `'1'`, `'999'`) as durability level values and report a clear error instead.
| var exception = Assert.Throws<System.Management.Automation.PSInvalidOperationException>( | ||
| () => cmdlet.GetNodeTypeDurabilityLevel("invalid")); | ||
|
|
||
| Assert.Equal( | ||
| string.Format("Cannot parse durability level {0}. Valid values are Bronze, Silver, and Gold.", "invalid"), | ||
| exception.Message); | ||
| } |
Live test results — TestFx
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- src/ServiceFabric/ServiceFabric/Properties/Resources.Designer.cs: Generated file
Suppressed comments (1)
src/ServiceFabric/ServiceFabric.Test/ScenarioTests/DurabilityLevelTests.cs:86
- This test asserts the full English exception message verbatim, which is brittle (resource text/punctuation changes will break the test without changing behavior). Consider asserting on key substrings instead (or on the resource string if it can be referenced) to keep the regression coverage focused on behavior.
Assert.Equal(
string.Format("Cannot parse durability level {0}. Valid values are Bronze, Silver, and Gold.", "invalid"),
exception.Message);
| * Fixed `Update-AzServiceFabricDurability` and related cmdlets to reject numeric strings (e.g., `'1'`, `'999'`) as durability level values and report a clear error instead. | ||
|
|
Live test results — TestFx
|
There was a problem hiding this comment.
Agent Assist review — pass
All checks are green and the change is well-scoped.
CI
All 12 checks passed (0 failed, 0 pending).
Test validation
- Live test: Passed.
- Regression coverage: Not applicable to the changed files.
Summary
GetDurabilityLevel(string) now trims and case-insensitively parses the durability value (previously Enum.Parse was case-sensitive, so lowercase values like silver threw before ShouldProcess), while explicitly rejecting numeric strings with a clear error message. UpdateAzureRmServiceFabricNodeBase was updated to reuse the fixed helper instead of duplicating the case-sensitive Enum.Parse call. A new unit test (DurabilityLevelTests) covers the node-type and VMSS-extension code paths for all three valid values plus invalid/numeric input, and ChangeLog.md documents both fixes under the correct release header.
Risk assessment
11/100 · Low · High confidence
Owning-squad review is recommended for servicefabric before merge. Main signals: failure-handling behavior.
🤖 PR Validation — ️✔️ All clear
Update-AzServiceFabricDurabilityfailed beforeShouldProcesswhen existing node type or VMSS extension durability values used lowercase casing (for example,silver).Shared behavior
Coverage