Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4e6a04b
Fixed the Validation for bbs-shared-home and archive-path
AakashSuresh2003 Mar 4, 2025
ad1004a
Update[1327]: Fixed the Validation for bbs-shared-home and archive-path
AakashSuresh2003 Mar 5, 2025
110c30a
Update[1327]: Fixed the Validation for bbs-shared-home and archive-path
AakashSuresh2003 Mar 5, 2025
be2b9da
Update[1327]: Modified the code due to dotnet formate failure
AakashSuresh2003 Mar 5, 2025
d302f8b
Fix[1211] Refine validation and wrote test case for BbsSharedHome, Ar…
AakashSuresh2003 Mar 6, 2025
58bf290
Fix[1211] Refined the codebase and updated the testcases
AakashSuresh2003 Mar 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Octoshift/Services/FileSystemProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,6 @@ public virtual async Task CopySourceToTargetStreamAsync(Stream source, Stream ta
await source.CopyToAsync(target);
}
}

public virtual bool DirectoryExists(string path) => Directory.Exists(path);
}
4 changes: 2 additions & 2 deletions src/OctoshiftCLI.Tests/Octoshift/Services/AdoApiTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


_mockAdoClient.Verify(m => m.PostAsync(endpoint, It.Is<object>(y => y.ToJson() == payload.ToJson())).Result);
}
Expand Down Expand Up @@ -551,7 +551,7 @@ public async Task GetLastPushDate_Should_Return_LastPushDate()
{
value = new[]
{
new { date = expectedDate.ToShortDateString() }
new { date = expectedDate.ToString("o") }
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}
};

Expand Down
92 changes: 79 additions & 13 deletions src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,12 +251,15 @@ public async Task Happy_Path_Full_Flow_Running_On_Bbs_Server()
const string bbsSharedHome = "bbs-shared-home";
var archivePath = $"{bbsSharedHome}/data/migration/export/Bitbucket_export_{BBS_EXPORT_ID}.tar";

_mockGithubApi.Setup(x => x.DoesRepoExist(GITHUB_ORG, GITHUB_REPO).Result).Returns(false);
// Mock directory existence check
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: please avoid adding unnecessary comments as they add unnecessary noise\

Suggested change
// Mock directory existence check

_mockFileSystemProvider.Setup(m => m.DirectoryExists(bbsSharedHome)).Returns(true);

_mockGithubApi.Setup(x => x.DoesRepoExist(GITHUB_ORG, GITHUB_REPO)).ReturnsAsync(false);
_mockBbsApi.Setup(x => x.StartExport(BBS_PROJECT, BBS_REPO)).ReturnsAsync(BBS_EXPORT_ID);
_mockBbsApi.Setup(x => x.GetExport(BBS_EXPORT_ID)).ReturnsAsync(("COMPLETED", "The export is complete", 100));
_mockAzureApi.Setup(x => x.UploadToBlob(It.IsAny<string>(), It.IsAny<FileStream>())).ReturnsAsync(new Uri(ARCHIVE_URL));
_mockGithubApi.Setup(x => x.GetOrganizationId(GITHUB_ORG).Result).Returns(GITHUB_ORG_ID);
_mockGithubApi.Setup(x => x.CreateBbsMigrationSource(GITHUB_ORG_ID).Result).Returns(MIGRATION_SOURCE_ID);
_mockGithubApi.Setup(x => x.GetOrganizationId(GITHUB_ORG)).ReturnsAsync(GITHUB_ORG_ID);
_mockGithubApi.Setup(x => x.CreateBbsMigrationSource(GITHUB_ORG_ID)).ReturnsAsync(MIGRATION_SOURCE_ID);
Comment on lines +261 to +262
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


// Act
var args = new MigrateRepoCommandArgs
Expand All @@ -279,7 +282,7 @@ public async Task Happy_Path_Full_Flow_Running_On_Bbs_Server()
_mockGithubApi.Object,
_mockBbsApi.Object,
_mockEnvironmentVariableProvider.Object,
null, // in case of running on Bitbucket server, the downloader will be null
null, // In case of running on Bitbucket server, the downloader will be null
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

_mockAzureApi.Object,
_mockAwsApi.Object,
_mockFileSystemProvider.Object,
Expand Down Expand Up @@ -355,17 +358,19 @@ public async Task Happy_Path_Uploads_To_Github_Storage()

await using var gitContentStream = new MemoryStream(gitArchiveContents.ToBytes());

// Mock file system to return a valid file
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

Suggested change
// Mock file system to return a valid file

_mockFileSystemProvider.Setup(m => m.FileExists(gitArchiveFilePath)).Returns(true);
_mockFileSystemProvider.Setup(m => m.OpenRead(gitArchiveFilePath)).Returns(gitContentStream);

_mockGithubApi.Setup(x => x.GetOrganizationId(GITHUB_ORG).Result).Returns(GITHUB_ORG_ID);
_mockGithubApi.Setup(x => x.CreateBbsMigrationSource(GITHUB_ORG_ID).Result).Returns(MIGRATION_SOURCE_ID);
_mockGithubApi.Setup(x => x.GetOrganizationDatabaseId(GITHUB_ORG).Result).Returns(githubOrgDatabaseId);
_mockGithubApi.Setup(x => x.GetOrganizationId(GITHUB_ORG)).ReturnsAsync(GITHUB_ORG_ID);
_mockGithubApi.Setup(x => x.CreateBbsMigrationSource(GITHUB_ORG_ID)).ReturnsAsync(MIGRATION_SOURCE_ID);
_mockGithubApi.Setup(x => x.GetOrganizationDatabaseId(GITHUB_ORG)).ReturnsAsync(githubOrgDatabaseId);
_mockGithubApi
.Setup(x => x.UploadArchiveToGithubStorage(
githubOrgDatabaseId,
It.IsAny<string>(),
It.Is<Stream>(s => (s as MemoryStream).ToArray().GetString() == gitArchiveContents)).Result)
.Returns(gitArchiveUrl);
It.Is<Stream>(s => (s as MemoryStream).ToArray().GetString() == gitArchiveContents)))
.ReturnsAsync(gitArchiveUrl);
Comment on lines +365 to +373
Copy link
Collaborator

Choose a reason for hiding this comment

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

same, please avoid unnecessary refactors


// Act
var args = new MigrateRepoCommandArgs
Expand Down Expand Up @@ -637,8 +642,10 @@ public async Task Uses_Archive_Path_If_Provided()
_mockEnvironmentVariableProvider.Setup(m => m.TargetGithubPersonalAccessToken(It.IsAny<bool>())).Returns(GITHUB_PAT);

var archiveBytes = Encoding.ASCII.GetBytes("here are some bytes");
_mockFileSystemProvider.Setup(x => x.ReadAllBytesAsync(ARCHIVE_PATH)).ReturnsAsync(archiveBytes);
// Mock file existence check
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Mock file existence check

_mockFileSystemProvider.Setup(x => x.FileExists(ARCHIVE_PATH)).Returns(true);

_mockFileSystemProvider.Setup(x => x.ReadAllBytesAsync(ARCHIVE_PATH)).ReturnsAsync(archiveBytes);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same, please avoid unnecessary refactors

_mockAzureApi.Setup(x => x.UploadToBlob(It.IsAny<string>(), It.IsAny<FileStream>())).ReturnsAsync(new Uri(ARCHIVE_URL));

_mockGithubApi.Setup(x => x.GetOrganizationId(GITHUB_ORG).Result).Returns(GITHUB_ORG_ID);
Expand Down Expand Up @@ -670,6 +677,7 @@ public async Task Uses_Archive_Path_If_Provided()
));
}


Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

[Fact]
public async Task Invoke_With_Bbs_Server_Url_Throws_When_Smb_User_Is_Provided_And_Smb_Password_Is_Not_Provided()
{
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Mock file existence check

_mockFileSystemProvider.Setup(m => m.FileExists(ARCHIVE_PATH)).Returns(true);

// Act
var args = new MigrateRepoCommandArgs
{
Expand Down Expand Up @@ -756,11 +767,13 @@ public async Task Uses_Aws_If_Credentials_Are_Passed()
// Arrange
_mockEnvironmentVariableProvider.Setup(m => m.TargetGithubPersonalAccessToken(It.IsAny<bool>())).Returns(GITHUB_PAT);

_mockGithubApi.Setup(x => x.GetOrganizationId(GITHUB_ORG).Result).Returns(GITHUB_ORG_ID);
_mockGithubApi.Setup(x => x.CreateBbsMigrationSource(GITHUB_ORG_ID).Result).Returns(MIGRATION_SOURCE_ID);
_mockGithubApi.Setup(x => x.GetOrganizationId(GITHUB_ORG)).ReturnsAsync(GITHUB_ORG_ID);
_mockGithubApi.Setup(x => x.CreateBbsMigrationSource(GITHUB_ORG_ID)).ReturnsAsync(MIGRATION_SOURCE_ID);
Comment on lines +770 to +771
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

Copy link
Author

Choose a reason for hiding this comment

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

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.

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

// Act
// Mock the file existence check
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Mock the file existence check

_mockFileSystemProvider.Setup(fs => fs.FileExists(ARCHIVE_PATH)).Returns(true);

var args = new MigrateRepoCommandArgs
{
GithubOrg = GITHUB_ORG,
Expand Down Expand Up @@ -960,5 +973,58 @@ public async Task Sets_Target_Repo_Visibility()
targetRepoVisibility
));
}
[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();
}
Comment on lines +976 to +1028
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

  1. It_Throws_When_Should_Upload_And_ArchivePath_Is_Invalid()
  2. It_Throws_When_Should_Download_And_BbsSharedHome_Is_Invalid() --> this is for when args.ShouldDownloadArchive() returns true
  3. 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

}
}
16 changes: 15 additions & 1 deletion src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ public async Task Handle(MigrateRepoCommandArgs args)
}

ValidateOptions(args);

var exportId = 0L;
var migrationSourceId = "";

Expand Down Expand Up @@ -384,5 +383,20 @@ private void ValidateUploadOptions(MigrateRepoCommandArgs args)
throw new OctoshiftCliException("Either --aws-region or AWS_REGION environment variable must be set.");
}
}
// Validate BbsSharedHome
if (!string.IsNullOrEmpty(args.BbsSharedHome) &&
args.ArchivePath.IsNullOrWhiteSpace() &&
args.BbsSharedHome.HasValue() &&
!_fileSystemProvider.DirectoryExists(args.BbsSharedHome))
Comment on lines +387 to +390
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

{
throw new OctoshiftCliException($"Invalid --bbs-shared-home path: '{args.BbsSharedHome}'. Directory does not exist.");
}

// Validate ArchivePath
if (!string.IsNullOrEmpty(args.ArchivePath) && !_fileSystemProvider.FileExists(args.ArchivePath))
Copy link
Collaborator

Choose a reason for hiding this comment

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

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))

Copy link
Author

Choose a reason for hiding this comment

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

Sure

{
throw new OctoshiftCliException($"Invalid --archive-path: '{args.ArchivePath}'. File does not exist.");
}

}
}
Loading