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

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 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
Contributor

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
Contributor

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
Contributor

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
94 changes: 81 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 @@
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
Contributor

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
Contributor

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
Contributor

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 @@
_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
Contributor

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

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

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

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
Contributor

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 @@
_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
Contributor

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
Contributor

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 @@
));
}


Copy link
Contributor

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 @@
// Arrange
_mockAzureApi.Setup(x => x.UploadToBlob(It.IsAny<string>(), It.IsAny<FileStream>())).ReturnsAsync(new Uri(ARCHIVE_URL));

// Mock file existence check
Copy link
Contributor

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 @@
// 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
Contributor

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
Contributor

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,60 @@
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
const string ARCHIVE_PATH = "valid/path/to/archive.tar";
_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";
const string ARCHIVE_PATH = "valid/path/to/archive.tar";

_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();
}
}
}
21 changes: 20 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 @@
}

ValidateOptions(args);

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

Expand Down Expand Up @@ -384,5 +383,25 @@
throw new OctoshiftCliException("Either --aws-region or AWS_REGION environment variable must be set.");
}
}
// Validate BbsSharedHome
if (!string.IsNullOrEmpty(args.BbsSharedHome))
{
if (args.ArchivePath.IsNullOrWhiteSpace()) // Removed the ShouldUploadArchive Since we are checking it already
{
if (args.BbsSharedHome.HasValue() && !_fileSystemProvider.DirectoryExists(args.BbsSharedHome))
{
throw new OctoshiftCliException($"Invalid --bbs-shared-home path: '{args.BbsSharedHome}'. Directory does not exist.");
}
}
}

// Validate ArchivePath
if (!string.IsNullOrEmpty(args.ArchivePath))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

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

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