Fixed the Validation for bbs-shared-home and archive-path #1327

AakashSuresh2003 · 2025-03-04T10:18:40Z

This PR refines the validation for BbsSharedHome and ArchivePath inputs.

Validation Enhancements:

Validated that the BbsSharedHome directory exists if provided and ArchivePath is not set.
Ensured that the ArchivePath file exists if provided.

Did you write/update appropriate tests
Release notes updated (if appropriate)
Appropriate logging output
Issue linked
Docs updated (or issue created)
New package licenses are added to ThirdPartyNotices.txt (if applicable)

github-actions · 2025-03-04T10:22:20Z

Unit Test Results

869 tests 869 ✅ 20s ⏱️
1 suites 0 💤
1 files 0 ❌

Results for commit 58bf290.

♻️ This comment has been updated with latest results.

ArinGhazarian

👋 Thanks for the contribution, other than the review notes, enough test coverage needs to get added for the changes and the PR should have a write up briefly explaining what it does.

ArinGhazarian · 2025-03-04T18:13:52Z

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

+    private void ValidateBbsSharedHome(MigrateRepoCommandArgs args)
+    {
+        if (!string.IsNullOrEmpty(args.BbsSharedHome))
+        {
+            if (IsRunningOnBitbucketServer())
+            {
+                if (!Directory.Exists(args.BbsSharedHome))
+                {
+                    throw new OctoshiftCliException($"Invalid --bbs-shared-home path: '{args.BbsSharedHome}'. Directory does not exist.");
+                }
+            }
+            else
+            {
+                if (!Directory.Exists(args.BbsSharedHome))
+                {
+                    throw new OctoshiftCliException($"Invalid --bbs-shared-home path: '{args.BbsSharedHome}'. Directory does not exist.");
+                }
+            }
+        }
+    }


A few things here:

This check should go into the ValidateUploadOptions() method.

We cannot validate the existence of the directory if the CLI is not running on the BB server because the path is on the server. So the else part won't work here.

The check to determine whether we're running on BB server is not reliable as it only check an env variable, instead we can do this:

if (args.ShouldUploadArchive() && args.ArchivePath.IsNullOrWhiteSpace()) { if (args.BbsSharedHome.HasValue() && !_fileSystemProvider.DirectoryExists(args.BbsSharedHome) { throw new OctoshiftCliException($"Invalid --bbs-shared-home path: '{args.BbsSharedHome}'. Directory does not exist."); }

Note the use of _fileSystemProvider.DirectoryExists(), instead of Directory.Exists(). You also need to add the DirectoryExists() method to the FileSystemProvider class.

Hi @ArinGhazarian,
I have updated the code and created a new pull request. Could you please review it and provide any suggestions?

ArinGhazarian · 2025-03-04T18:17:25Z

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

+    private void ValidateArchivePath(MigrateRepoCommandArgs args)
+    {
+        if (!string.IsNullOrEmpty(args.ArchivePath))
+        {
+            if (!File.Exists(args.ArchivePath))
+            {
+                throw new OctoshiftCliException($"Invalid --archive-path: '{args.ArchivePath}'. File does not exist.");
+            }
+            _log.LogInformation($"Archive path for upload: {args.ArchivePath}");
+        }
+    }


A few notes here too:

This check also needs to go under ValidateUploadOptions() method.

Instead of File.Exists, the _fileSystemProvider.FileExists() abstraction should be used. This will simplify unit testing.

There is no need to add an extra info log as all args will get logged automatically.

ArinGhazarian · 2025-03-04T18:18:55Z

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

@@ -1,4 +1,5 @@
 using System;
+using System.IO;


We don't have a direct dependency to System.IO, instead we use an abstraction FileSystemProvider.

ArinGhazarian · 2025-03-04T18:19:59Z

src/OctoshiftCLI.Tests/ado2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs

@@ -277,6 +277,36 @@ public async Task It_Falls_Back_To_Ado_And_Github_Pats_From_Environment_When_Not
        _mockEnvironmentVariableProvider.Verify(m => m.TargetGithubPersonalAccessToken(It.IsAny<bool>()));
    }

+    [Fact]
+    public async Task Throws_Decorated_Error_When_Create_Migration_Source_Fails_With_Permissions_Error_New()


Please remove this test as it doesn't test any of the changes introduced in this PR.

ArinGhazarian

👋 As I mentioned in my previous review the following has to be done to be able to merge the PR:

Add test coverage. Two new tests should be added to cover both validation scenarios
CI should be green. There are some failing tests because of the two newly added validation. To fix them you just need to mock the FileExists and DirectoryExists methods (e.g. _mockFileSystemProvider.Setup(x => x.FileExists(gitArchiveFilePath)).Returns(true);)
PR needs to have a writeup briefly explaining what it does.

Please request a re-review once all of the above items are done.

ArinGhazarian · 2025-03-05T17:57:40Z

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

+        // Validate BbsSharedHome
+        if (!string.IsNullOrEmpty(args.BbsSharedHome))
+        {
+            if (args.ShouldUploadArchive() && args.ArchivePath.IsNullOrWhiteSpace())


Since we're already in ValidateUploadOptions there is not need to check for ShouldUploadArchive:

Suggested change

// Validate BbsSharedHome

if (!string.IsNullOrEmpty(args.BbsSharedHome))

{

if (args.ShouldUploadArchive() && args.ArchivePath.IsNullOrWhiteSpace())

if (args.BbsSharedHome.HasValue())

{

if (args.ArchivePath.IsNullOrWhiteSpace())

ArinGhazarian · 2025-03-05T17:58:54Z

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

+        // Validate ArchivePath
+        if (!string.IsNullOrEmpty(args.ArchivePath))


nit

Suggested change

// Validate ArchivePath

if (!string.IsNullOrEmpty(args.ArchivePath))

if (args.ArchivePath.HasValue())

…chivePath and also updated the failing test cases

src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

github-actions · 2025-03-06T09:38:26Z

Package	Line Rate	Branch Rate	Complexity	Health
gei	81%	73%	596	✔
ado2gh	84%	78%	631	✔
bbs2gh	82%	76%	681	✔
Octoshift	87%	74%	1403	✔
Summary	84% (7200 / 8549)	75% (1685 / 2246)	3311	✔

AakashSuresh2003 · 2025-03-06T09:42:19Z

PR Summary:

This PR refines the validation for BbsSharedHome and ArchivePath inputs.

Validation Enhancements:

Validated that the BbsSharedHome directory exists if provided and ArchivePath is not set.
Ensured that the ArchivePath file exists if provided.

Unit Tests:

Added unit tests to verify that no exception is thrown when:
- BbsSharedHome exists.
- ArchivePath exists.
- Both BbsSharedHome and ArchivePath are valid.

ArinGhazarian

Please revert all unnecessary refactors to keep the scope of changes as minimal and relevant as possible.

ArinGhazarian · 2025-03-06T19:34:43Z

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

+        if (!string.IsNullOrEmpty(args.BbsSharedHome) &&
+            args.ArchivePath.IsNullOrWhiteSpace() &&
+            args.BbsSharedHome.HasValue() &&
+            !_fileSystemProvider.DirectoryExists(args.BbsSharedHome))


Also regarding this check I missed the fact that the BBS shared home is also used when downloading the archive (my bad sorry). So this check should actually go into the ValidateOptions() method and the if clause needs to change a bit to account for either when downloading or uploading from the BBS instance:

if (args.ShouldDownloadArchive() || (args.ShouldUploadArchive() && args.ArchivePath.IsNullOrWhiteSpace())) { if (args.BbsSharedHome.HasValue() && !_fileSystemProvider.DirectoryExists(args.BbsSharedHome)) { throw new OctoshiftCliException($"Invalid --bbs-shared-home path: '{args.BbsSharedHome}'. Directory does not exist."); } }

the check makes sure that we're going to validate the BbsSharedHome only if we need to download the archive or we need to upload and running the cli on the BB instance.

ArinGhazarian · 2025-03-06T19:08:01Z

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

+        }
+
+        // Validate ArchivePath
+        if (!string.IsNullOrEmpty(args.ArchivePath) && !_fileSystemProvider.FileExists(args.ArchivePath))


nit: use the extension method HasValue() for a slightly better readability

Suggested change

if (!string.IsNullOrEmpty(args.ArchivePath) && !_fileSystemProvider.FileExists(args.ArchivePath))

if (args.ArchivePath.HasValue() && !_fileSystemProvider.FileExists(args.ArchivePath))

ArinGhazarian · 2025-03-06T19:15:46Z

src/OctoshiftCLI.Tests/Octoshift/Services/AdoApiTests.cs

@@ -455,7 +455,7 @@ public async Task AddRepoToBoardsGithubConnection_Should_Send_Correct_Payload()
            }
        };

-        await sut.AddRepoToBoardsGithubConnection(ADO_ORG, ADO_TEAM_PROJECT, connectionId, connectionName, endpointId, new List<string>() { ADO_REPO, repo2 });
+        await sut.AddRepoToBoardsGithubConnection(ADO_ORG, ADO_TEAM_PROJECT, connectionId, connectionName, endpointId, [ADO_REPO, repo2]);


Please avoid doing any refactors in this PR to keep the scope of changes as small as possible.

ArinGhazarian · 2025-03-06T19:16:18Z

src/OctoshiftCLI.Tests/Octoshift/Services/AdoApiTests.cs

@@ -551,7 +551,7 @@ public async Task GetLastPushDate_Should_Return_LastPushDate()
        {
            value = new[]
            {
-                new { date = expectedDate.ToShortDateString() }
+                new { date = expectedDate.ToString("o") }


This caused an error

OctoshiftCLI.Tests.Octoshift.Services.AdoApiTests.GetLastPushDate_Should_Return_LastPushDate [9 ms]
Error Message:
System.FormatException : String '14-02-2022' was not recognized as a valid DateTime.

Solution is the JSON response must provide a date in a standardized format like "yyyy-MM-ddTHH:mm:ssZ". Then only the test case is passed.

the test passes on CI so there must be some issue on your local env.

ArinGhazarian · 2025-03-06T19:47:38Z

src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs

+            _mockGithubApi.Setup(x => x.GetOrganizationId(GITHUB_ORG)).ReturnsAsync(GITHUB_ORG_ID);
+            _mockGithubApi.Setup(x => x.CreateBbsMigrationSource(GITHUB_ORG_ID)).ReturnsAsync(MIGRATION_SOURCE_ID);


This change is not necessary, as described here Returns is the preferred way. Please avoid doing unnecessary refactors.

Error Message:
OctoshiftCLI.OctoshiftCliException : Invalid --archive-path: 'path/to/archive.tar'. File does not exist.

Solution I used:
Since the migration command is validating the file existence, we need to mock _fileSystemProvider.FileExists(ARCHIVE_PATH) to return true in the test setup.

this comment was about reverting the refactor which wasn't necessary.

ArinGhazarian · 2025-03-06T19:56:32Z

src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs

@@ -670,6 +677,7 @@ public async Task Uses_Archive_Path_If_Provided()
            ));
        }

+


Suggested change

ArinGhazarian · 2025-03-06T19:57:51Z

src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs

@@ -715,6 +723,9 @@ public async Task It_Does_Not_Set_The_Archive_Path_When_Archive_Path_Is_Provided
            // Arrange
            _mockAzureApi.Setup(x => x.UploadToBlob(It.IsAny<string>(), It.IsAny<FileStream>())).ReturnsAsync(new Uri(ARCHIVE_URL));

+            // Mock file existence check


Suggested change

// Mock file existence check

ArinGhazarian · 2025-03-06T19:59:07Z

src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs

+            _mockGithubApi.Setup(x => x.GetOrganizationId(GITHUB_ORG)).ReturnsAsync(GITHUB_ORG_ID);
+            _mockGithubApi.Setup(x => x.CreateBbsMigrationSource(GITHUB_ORG_ID)).ReturnsAsync(MIGRATION_SOURCE_ID);


Solution I used:
Since the migration command is validating the file existence, we need to mock _fileSystemProvider.FileExists(ARCHIVE_PATH) to return true in the test setup.

ArinGhazarian · 2025-03-06T19:59:17Z

src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs

            _mockAwsApi.Setup(x => x.UploadToBucket(AWS_BUCKET_NAME, ARCHIVE_PATH, It.IsAny<string>())).ReturnsAsync(ARCHIVE_URL);

-            // Act
+            // Mock the file existence check


Suggested change

// Mock the file existence check

ArinGhazarian · 2025-03-06T20:08:47Z

src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs

+        [Fact]
+        public async Task Does_Not_Throw_Exception_If_BbsSharedHome_Exists()
+        {
+            // Arrange
+            const string BBS_SHARED_HOME = "valid/path";
+            _mockFileSystemProvider.Setup(x => x.DirectoryExists(BBS_SHARED_HOME)).Returns(true);
+
+            var args = new MigrateRepoCommandArgs
+            {
+                BbsSharedHome = BBS_SHARED_HOME,
+                ArchivePath = string.Empty
+            };
+
+            // Act & Assert
+            await FluentActions.Invoking(() => _handler.Handle(args))
+                .Should().NotThrowAsync();
+        }
+
+        [Fact]
+        public async Task Does_Not_Throw_Exception_If_ArchivePath_Exists()
+        {
+            // Arrange
+            _mockFileSystemProvider.Setup(x => x.FileExists(ARCHIVE_PATH)).Returns(true);
+
+            var args = new MigrateRepoCommandArgs
+            {
+                ArchivePath = ARCHIVE_PATH
+            };
+
+            // Act & Assert
+            await FluentActions.Invoking(() => _handler.Handle(args))
+                .Should().NotThrowAsync();
+        }
+
+        [Fact]
+        public async Task Does_Not_Throw_Exception_If_BbsSharedHome_And_ArchivePath_Are_Both_Valid()
+        {
+            // Arrange
+            const string BBS_SHARED_HOME = "valid/path";
+
+            _mockFileSystemProvider.Setup(x => x.DirectoryExists(BBS_SHARED_HOME)).Returns(true);
+            _mockFileSystemProvider.Setup(x => x.FileExists(ARCHIVE_PATH)).Returns(true);
+
+            var args = new MigrateRepoCommandArgs
+            {
+                BbsSharedHome = BBS_SHARED_HOME,
+                ArchivePath = ARCHIVE_PATH
+            };
+
+            // Act & Assert
+            await FluentActions.Invoking(() => _handler.Handle(args))
+                .Should().NotThrowAsync();
+        }


These tests' logic should be reverted. Because validations are checking exceptional cases, the tests should also check those cases. So here we only need 3 tests:

It_Throws_When_Should_Upload_And_ArchivePath_Is_Invalid()

It_Throws_When_Should_Download_And_BbsSharedHome_Is_Invalid() --> this is for when args.ShouldDownloadArchive() returns true

It_Throws_When_Should_Upload_And_Running_On_Bbs_Instance_And_BbsSharedHome_Is_Invalid() --> this is for when args.ShouldUploadArchive && args.ArchivePath.IsNullOrWhiteSpace() returns true

Fixed the Validation for bbs-shared-home and archive-path

4e6a04b

jeviet requested review from ArinGhazarian and synthead March 4, 2025 15:41

ArinGhazarian requested changes Mar 4, 2025

View reviewed changes

AakashSuresh2003 added 2 commits March 5, 2025 10:06

Update[1327]: Fixed the Validation for bbs-shared-home and archive-path

ad1004a

Update[1327]: Fixed the Validation for bbs-shared-home and archive-path

110c30a

AakashSuresh2003 requested a review from ArinGhazarian March 5, 2025 04:45

Update[1327]: Modified the code due to dotnet formate failure

be2b9da

ArinGhazarian requested changes Mar 5, 2025

View reviewed changes

Fix[1211] Refine validation and wrote test case for BbsSharedHome, Ar…

d302f8b

…chivePath and also updated the failing test cases

github-advanced-security bot found potential problems Mar 6, 2025

View reviewed changes

Fix[1211] Refined the codebase and updated the testcases

58bf290

AakashSuresh2003 requested a review from ArinGhazarian March 6, 2025 09:43

ArinGhazarian requested changes Mar 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed the Validation for bbs-shared-home and archive-path #1327

Fixed the Validation for bbs-shared-home and archive-path #1327

AakashSuresh2003 commented Mar 4, 2025 •

edited by ArinGhazarian

Loading

github-actions bot commented Mar 4, 2025 •

edited

Loading

ArinGhazarian left a comment

ArinGhazarian Mar 4, 2025

AakashSuresh2003 Mar 5, 2025

ArinGhazarian Mar 4, 2025

ArinGhazarian Mar 4, 2025

ArinGhazarian Mar 4, 2025

AakashSuresh2003 Mar 5, 2025

ArinGhazarian left a comment

ArinGhazarian Mar 5, 2025

ArinGhazarian Mar 5, 2025

github-actions bot commented Mar 6, 2025

AakashSuresh2003 commented Mar 6, 2025

ArinGhazarian left a comment

ArinGhazarian Mar 6, 2025

ArinGhazarian Mar 6, 2025

AakashSuresh2003 Mar 6, 2025

ArinGhazarian Mar 6, 2025

ArinGhazarian Mar 6, 2025

AakashSuresh2003 Mar 6, 2025

ArinGhazarian Mar 7, 2025

ArinGhazarian Mar 6, 2025

AakashSuresh2003 Mar 6, 2025

ArinGhazarian Mar 7, 2025

ArinGhazarian Mar 6, 2025

ArinGhazarian Mar 6, 2025

ArinGhazarian Mar 6, 2025

AakashSuresh2003 Mar 6, 2025

ArinGhazarian Mar 6, 2025

ArinGhazarian Mar 6, 2025

		// Validate ArchivePath
		if (!string.IsNullOrEmpty(args.ArchivePath))

	// Validate ArchivePath
	if (!string.IsNullOrEmpty(args.ArchivePath))
	if (args.ArchivePath.HasValue())

	if (!string.IsNullOrEmpty(args.ArchivePath) && !_fileSystemProvider.FileExists(args.ArchivePath))
	if (args.ArchivePath.HasValue() && !_fileSystemProvider.FileExists(args.ArchivePath))

		_mockGithubApi.Setup(x => x.GetOrganizationId(GITHUB_ORG)).ReturnsAsync(GITHUB_ORG_ID);
		_mockGithubApi.Setup(x => x.CreateBbsMigrationSource(GITHUB_ORG_ID)).ReturnsAsync(MIGRATION_SOURCE_ID);

		@@ -670,6 +677,7 @@ public async Task Uses_Archive_Path_If_Provided()
		));
		}

Fixed the Validation for bbs-shared-home and archive-path #1327

Are you sure you want to change the base?

Fixed the Validation for bbs-shared-home and archive-path #1327

Conversation

AakashSuresh2003 commented Mar 4, 2025 • edited by ArinGhazarian Loading

Validation Enhancements:

github-actions bot commented Mar 4, 2025 • edited Loading

Unit Test Results

ArinGhazarian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArinGhazarian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 6, 2025

AakashSuresh2003 commented Mar 6, 2025

PR Summary:

Validation Enhancements:

Unit Tests:

ArinGhazarian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AakashSuresh2003 commented Mar 4, 2025 •

edited by ArinGhazarian

Loading

github-actions bot commented Mar 4, 2025 •

edited

Loading