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 1 commit
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
30 changes: 30 additions & 0 deletions src/OctoshiftCLI.Tests/ado2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Sure

{
// Arrange
_mockEnvironmentVariableProvider
.Setup(m => m.TargetGithubPersonalAccessToken(It.IsAny<bool>()))
.Returns(GITHUB_TOKEN);
_mockEnvironmentVariableProvider
.Setup(m => m.AdoPersonalAccessToken(It.IsAny<bool>()))
.Returns(ADO_TOKEN);

_mockGithubApi.Setup(x => x.GetOrganizationId(GITHUB_ORG).Result).Returns(GITHUB_ORG_ID);
_mockGithubApi
.Setup(x => x.CreateAdoMigrationSource(GITHUB_ORG_ID, null).Result)
.Throws(new OctoshiftCliException("monalisa does not have the correct permissions to execute `CreateMigrationSource`"));

// Act
await _handler.Invoking(async x => await x.Handle(new MigrateRepoCommandArgs
{
AdoOrg = ADO_ORG,
AdoTeamProject = ADO_TEAM_PROJECT,
AdoRepo = ADO_REPO,
GithubOrg = GITHUB_ORG,
GithubRepo = GITHUB_REPO,
}))
.Should()
.ThrowAsync<OctoshiftCliException>()
.WithMessage($"monalisa does not have the correct permissions to execute `CreateMigrationSource`. Please check that:\n (a) you are a member of the `{GITHUB_ORG}` organization,\n (b) you are an organization owner or you have been granted the migrator role and\n (c) your personal access token has the correct scopes.\nFor more information, see https://docs.github.com/en/migrations/using-github-enterprise-importer/preparing-to-migrate-with-github-enterprise-importer/managing-access-for-github-enterprise-importer.");
}

[Fact]
public async Task Sets_Target_Repo_Visibility_When_Specified()
{
Expand Down
44 changes: 43 additions & 1 deletion src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.IO;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

using System.Runtime.InteropServices;
using System.Threading.Tasks;
using OctoshiftCLI.BbsToGithub.Services;
Expand Down Expand Up @@ -51,7 +52,8 @@ public async Task Handle(MigrateRepoCommandArgs args)
}

ValidateOptions(args);

ValidateBbsSharedHome(args);
ValidateArchivePath(args);
var exportId = 0L;
var migrationSourceId = "";

Expand Down Expand Up @@ -123,6 +125,46 @@ public async Task Handle(MigrateRepoCommandArgs args)
}
}

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.");
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A few things here:

  1. This check should go into the ValidateUploadOptions() method.
  2. 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.
  3. 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.

Copy link
Author

Choose a reason for hiding this comment

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

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


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}");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A few notes here too:

  1. This check also needs to go under ValidateUploadOptions() method.
  2. Instead of File.Exists, the _fileSystemProvider.FileExists() abstraction should be used. This will simplify unit testing.
  3. There is no need to add an extra info log as all args will get logged automatically.



private bool IsRunningOnBitbucketServer()
{
return Environment.GetEnvironmentVariable("BITBUCKET_SERVER") != null;
}


private string GetSourceExportArchiveAbsolutePath(string bbsSharedHomeDirectory, long exportId)
{
if (bbsSharedHomeDirectory.IsNullOrWhiteSpace())
Expand Down
Loading