From 9a240b6b053665d4b1a9cdee820fbd2e7e8ea162 Mon Sep 17 00:00:00 2001 From: Tiago Pascoal Date: Wed, 30 Jul 2025 18:54:30 +0100 Subject: [PATCH 1/6] Add support for overriding multipart upload chunk size via environment variable Allows users to set an env variable GITHUB_OWNED_STORAGE_MULTIPART_BYTES with the number of bytes (min 5 Mib) for max number of bytes senter for each part in a multipart upload. By allowing the users to use a smaller size than the default, allows the upload to work in cases a user has a proxy that mishandles or blocks big uploads or for very slow connections. If not defined uses the default 100 Mib value --- RELEASENOTES.md | 2 +- src/Octoshift/Services/ArchiveUploader.cs | 21 +++ .../Services/ArchiveUploadersTests.cs | 168 +++++++++++++++++- 3 files changed, 184 insertions(+), 7 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 8b1378917..b6c407bff 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -1 +1 @@ - +- Allow overriding the multipart upload chunk size when using GitHub owned storage. Set the value of the chunk (min 5Mib) in bytes in a environment variable called GITHUB_OWNED_STORAGE_MULTIPART_BYTES (default value 100Mib). Only set this if you a proxy that doesn't allow the default size upload or if you are on a very slow connection. diff --git a/src/Octoshift/Services/ArchiveUploader.cs b/src/Octoshift/Services/ArchiveUploader.cs index 78e2a3112..12369bcd0 100644 --- a/src/Octoshift/Services/ArchiveUploader.cs +++ b/src/Octoshift/Services/ArchiveUploader.cs @@ -15,6 +15,7 @@ public class ArchiveUploader private readonly GithubClient _client; private readonly OctoLogger _log; internal int _streamSizeLimit = 100 * 1024 * 1024; // 100 MiB + private const int MIN_MULTIPART_BYTES = 5 * 1024 * 1024; // 5 MiB minimum size for multipart upload. Don't allow overrides smaller than this. private readonly RetryPolicy _retryPolicy; private const string BASE_URL = "https://uploads.github.com"; @@ -24,6 +25,8 @@ public ArchiveUploader(GithubClient client, OctoLogger log, RetryPolicy retryPol _client = client; _log = log; _retryPolicy = retryPolicy; + + SetStreamSizeLimitFromEnvironment(); } public virtual async Task Upload(Stream archiveContent, string archiveName, string orgDatabaseId) { @@ -160,4 +163,22 @@ private Uri GetNextUrl(IEnumerable>> he } throw new OctoshiftCliException("Location header is missing in the response, unable to retrieve next URL for multipart upload."); } + + private void SetStreamSizeLimitFromEnvironment() + { + var envValue = Environment.GetEnvironmentVariable("GITHUB_OWNED_STORAGE_MULTIPART_BYTES"); + if (!int.TryParse(envValue, out var limit) || limit <= 0) + { + return; + } + + if (limit < MIN_MULTIPART_BYTES) + { + _log.LogWarning($"GITHUB_OWNED_STORAGE_MULTIPART_BYTES is set to {limit} bytes, but the minimum value is {MIN_MULTIPART_BYTES} bytes. Using default value of {_streamSizeLimit} bytes."); + return; + } + + _streamSizeLimit = limit; + _log.LogInformation($"Stream size limit set to {_streamSizeLimit} bytes."); + } } diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs index b47b302c9..854088c43 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs @@ -11,11 +11,12 @@ namespace OctoshiftCLI.Tests.Octoshift.Services; -public class ArchiveUploaderTests +public class ArchiveUploaderTests : IDisposable { private readonly Mock _githubClientMock; private readonly Mock _logMock; private readonly ArchiveUploader _archiveUploader; + private const string ENV_VAR_NAME = "GITHUB_OWNED_STORAGE_MULTIPART_BYTES"; public ArchiveUploaderTests() { @@ -25,6 +26,12 @@ public ArchiveUploaderTests() _archiveUploader = new ArchiveUploader(_githubClientMock.Object, _logMock.Object, retryPolicy); } + public void Dispose() + { + // Clean up environment variable after each test + Environment.SetEnvironmentVariable(ENV_VAR_NAME, null); + } + [Fact] public async Task Upload_Should_Throw_ArgumentNullException_When_Archive_Content_Is_Null() { @@ -37,6 +44,156 @@ public async Task Upload_Should_Throw_ArgumentNullException_When_Archive_Content await Assert.ThrowsAsync(() => _archiveUploader.Upload(nullStream, archiveName, orgDatabaseId)); } + [Fact] + public void Constructor_Should_Use_Valid_Environment_Variable_Value() + { + // Arrange + var customSize = 10 * 1024 * 1024; // 10 MiB + Environment.SetEnvironmentVariable(ENV_VAR_NAME, customSize.ToString()); + + var logMock = TestHelpers.CreateMock(); + var githubClientMock = TestHelpers.CreateMock(); + var retryPolicy = new RetryPolicy(logMock.Object); + + // Act + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + + // Assert + archiveUploader._streamSizeLimit.Should().Be(customSize); + logMock.Verify(x => x.LogInformation($"Stream size limit set to {customSize} bytes."), Times.Once); + } + + [Fact] + public void Constructor_Should_Use_Default_When_Environment_Variable_Not_Set() + { + // Arrange + Environment.SetEnvironmentVariable(ENV_VAR_NAME, null); + var defaultSize = 100 * 1024 * 1024; // 100 MiB + + var logMock = TestHelpers.CreateMock(); + var githubClientMock = TestHelpers.CreateMock(); + var retryPolicy = new RetryPolicy(logMock.Object); + + // Act + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + + // Assert + archiveUploader._streamSizeLimit.Should().Be(defaultSize); + } + + [Fact] + public void Constructor_Should_Use_Default_When_Environment_Variable_Is_Invalid() + { + // Arrange + Environment.SetEnvironmentVariable(ENV_VAR_NAME, "invalid_value"); + var defaultSize = 100 * 1024 * 1024; // 100 MiB + + var logMock = TestHelpers.CreateMock(); + var githubClientMock = TestHelpers.CreateMock(); + var retryPolicy = new RetryPolicy(logMock.Object); + + // Act + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + + // Assert + archiveUploader._streamSizeLimit.Should().Be(defaultSize); + } + + [Fact] + public void Constructor_Should_Use_Default_When_Environment_Variable_Is_Zero() + { + // Arrange + Environment.SetEnvironmentVariable(ENV_VAR_NAME, "0"); + var defaultSize = 100 * 1024 * 1024; // 100 MiB + + var logMock = TestHelpers.CreateMock(); + var githubClientMock = TestHelpers.CreateMock(); + var retryPolicy = new RetryPolicy(logMock.Object); + + // Act + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + + // Assert + archiveUploader._streamSizeLimit.Should().Be(defaultSize); + } + + [Fact] + public void Constructor_Should_Use_Default_When_Environment_Variable_Is_Negative() + { + // Arrange + Environment.SetEnvironmentVariable(ENV_VAR_NAME, "-1000"); + var defaultSize = 100 * 1024 * 1024; // 100 MiB + + var logMock = TestHelpers.CreateMock(); + var githubClientMock = TestHelpers.CreateMock(); + var retryPolicy = new RetryPolicy(logMock.Object); + + // Act + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + + // Assert + archiveUploader._streamSizeLimit.Should().Be(defaultSize); + } + + [Fact] + public void Constructor_Should_Use_Default_And_Log_Warning_When_Environment_Variable_Below_Minimum() + { + // Arrange + var belowMinimumSize = 1024 * 1024; // 1 MiB (below 5 MiB minimum) + var defaultSize = 100 * 1024 * 1024; // 100 MiB + var minSize = 5 * 1024 * 1024; // 5 MiB minimum + Environment.SetEnvironmentVariable(ENV_VAR_NAME, belowMinimumSize.ToString()); + + var logMock = TestHelpers.CreateMock(); + var githubClientMock = TestHelpers.CreateMock(); + var retryPolicy = new RetryPolicy(logMock.Object); + + // Act + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + + // Assert + archiveUploader._streamSizeLimit.Should().Be(defaultSize); + logMock.Verify(x => x.LogWarning($"GITHUB_OWNED_STORAGE_MULTIPART_BYTES is set to {belowMinimumSize} bytes, but the minimum value is {minSize} bytes. Using default value of {defaultSize} bytes."), Times.Once); + } + + [Fact] + public void Constructor_Should_Accept_Value_Equal_To_Minimum() + { + // Arrange + var minimumSize = 5 * 1024 * 1024; // 5 MiB minimum + Environment.SetEnvironmentVariable(ENV_VAR_NAME, minimumSize.ToString()); + + var logMock = TestHelpers.CreateMock(); + var githubClientMock = TestHelpers.CreateMock(); + var retryPolicy = new RetryPolicy(logMock.Object); + + // Act + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + + // Assert + archiveUploader._streamSizeLimit.Should().Be(minimumSize); + logMock.Verify(x => x.LogInformation($"Stream size limit set to {minimumSize} bytes."), Times.Once); + } + + [Fact] + public void Constructor_Should_Accept_Large_Valid_Value() + { + // Arrange + var largeSize = 500 * 1024 * 1024; // 500 MiB + Environment.SetEnvironmentVariable(ENV_VAR_NAME, largeSize.ToString()); + + var logMock = TestHelpers.CreateMock(); + var githubClientMock = TestHelpers.CreateMock(); + var retryPolicy = new RetryPolicy(logMock.Object); + + // Act + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + + // Assert + archiveUploader._streamSizeLimit.Should().Be(largeSize); + logMock.Verify(x => x.LogInformation($"Stream size limit set to {largeSize} bytes."), Times.Once); + } + [Fact] public async Task Upload_Should_Upload_All_Chunks_When_Stream_Exceeds_Limit() { @@ -128,8 +285,7 @@ public async Task Upload_Should_Retry_Failed_Upload_Part_Patch_Requests() .ThrowsAsync(new TimeoutException("The operation was canceled.")) .ThrowsAsync(new TimeoutException("The operation was canceled.")) .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [secondUploadUrl]) })); - - _githubClientMock // second PATCH request + _githubClientMock // second PATCH request .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{secondUploadUrl}", It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 3 }.ToJson()), null)) .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [lastUrl]) })); @@ -183,7 +339,7 @@ public async Task Upload_Should_Retry_Failed_Start_Upload_Post_Request() _githubClientMock // first PATCH request .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{firstUploadUrl}", It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 1, 2 }.ToJson()), null)) - .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [secondUploadUrl]) })); + .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [secondUploadUrl]) })); _githubClientMock // second PATCH request .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{secondUploadUrl}", @@ -237,9 +393,9 @@ public async Task Upload_Should_Retry_Failed_Complete_Upload_Put_Request() _githubClientMock // first PATCH request .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{firstUploadUrl}", It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 1, 2 }.ToJson()), null)) - .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [secondUploadUrl]) })); + .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [secondUploadUrl]) })); - _githubClientMock // second PATCH request + _githubClientMock // second PATCH request .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{secondUploadUrl}", It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 3 }.ToJson()), null)) .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [lastUrl]) })); From 234e76904149770958408128642054b1934ded03 Mon Sep 17 00:00:00 2001 From: Tiago Pascoal Date: Wed, 30 Jul 2025 19:25:53 +0100 Subject: [PATCH 2/6] Improve release note --- RELEASENOTES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index b6c407bff..26b227b62 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -1 +1 @@ -- Allow overriding the multipart upload chunk size when using GitHub owned storage. Set the value of the chunk (min 5Mib) in bytes in a environment variable called GITHUB_OWNED_STORAGE_MULTIPART_BYTES (default value 100Mib). Only set this if you a proxy that doesn't allow the default size upload or if you are on a very slow connection. +- Added support for configurable multipart upload chunk size for GitHub-owned storage uploads via `GITHUB_OWNED_STORAGE_MULTIPART_BYTES` environment variable (minimum 5 MiB, default 100 MiB) to improve upload reliability in environments with proxies or slow connections \ No newline at end of file From fd49f0222e640d97f0322c7a2b2f7d8a857fdcdb Mon Sep 17 00:00:00 2001 From: Tiago Pascoal Date: Wed, 30 Jul 2025 19:36:29 +0100 Subject: [PATCH 3/6] Fix warnings --- .../Services/ArchiveUploadersTests.cs | 34 +++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs index 854088c43..2dbed86b7 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs @@ -13,6 +13,8 @@ namespace OctoshiftCLI.Tests.Octoshift.Services; public class ArchiveUploaderTests : IDisposable { + private bool _disposed; + private readonly Mock _githubClientMock; private readonly Mock _logMock; private readonly ArchiveUploader _archiveUploader; @@ -26,10 +28,22 @@ public ArchiveUploaderTests() _archiveUploader = new ArchiveUploader(_githubClientMock.Object, _logMock.Object, retryPolicy); } + protected virtual void Dispose(bool disposing) + { + if (!_disposed) + { + if (disposing) + { + // Clean up environment variable after each test + Environment.SetEnvironmentVariable(ENV_VAR_NAME, null); + } + _disposed = true; + } + } public void Dispose() { - // Clean up environment variable after each test - Environment.SetEnvironmentVariable(ENV_VAR_NAME, null); + Dispose(true); + GC.SuppressFinalize(this); } [Fact] @@ -285,10 +299,10 @@ public async Task Upload_Should_Retry_Failed_Upload_Part_Patch_Requests() .ThrowsAsync(new TimeoutException("The operation was canceled.")) .ThrowsAsync(new TimeoutException("The operation was canceled.")) .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [secondUploadUrl]) })); - _githubClientMock // second PATCH request - .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{secondUploadUrl}", - It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 3 }.ToJson()), null)) - .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [lastUrl]) })); + _githubClientMock // second PATCH request + .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{secondUploadUrl}", + It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 3 }.ToJson()), null)) + .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [lastUrl]) })); // Mocking the final PUT request to complete the multipart upload _githubClientMock @@ -395,10 +409,10 @@ public async Task Upload_Should_Retry_Failed_Complete_Upload_Put_Request() It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 1, 2 }.ToJson()), null)) .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [secondUploadUrl]) })); - _githubClientMock // second PATCH request - .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{secondUploadUrl}", - It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 3 }.ToJson()), null)) - .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [lastUrl]) })); + _githubClientMock // second PATCH request + .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{secondUploadUrl}", + It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 3 }.ToJson()), null)) + .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [lastUrl]) })); // Mocking the final PUT request to complete the multipart upload _githubClientMock From fa462b5cf932dfdda0b72829274d4e5c52e9a323 Mon Sep 17 00:00:00 2001 From: Tiago Pascoal Date: Wed, 30 Jul 2025 22:04:42 +0100 Subject: [PATCH 4/6] Isolate archive uploader tests --- .../Octoshift/Services/ArchiveUploadersTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs index 2dbed86b7..df64ee143 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs @@ -11,6 +11,7 @@ namespace OctoshiftCLI.Tests.Octoshift.Services; +[Collection("Environment Variables")] public class ArchiveUploaderTests : IDisposable { private bool _disposed; From 7a93be3a422ba4cbbac98164220d650d60db6cdb Mon Sep 17 00:00:00 2001 From: Tiago Pascoal Date: Wed, 30 Jul 2025 23:27:34 +0100 Subject: [PATCH 5/6] Refactor ArchiveUploader to use EnvironmentVariableProvider ArchiveUploader no longer reads the env variable directly. This eliminates the race condition in the tests --- src/Octoshift/Factories/GithubApiFactory.cs | 6 +- src/Octoshift/Services/ArchiveUploader.cs | 9 +- .../Services/EnvironmentVariableProvider.cs | 4 + .../BbsToGithub.cs | 3 +- .../GhesToGithub.cs | 3 +- .../Services/ArchiveUploadersTests.cs | 118 +++++++++--------- .../Octoshift/Services/GithubApiTests.cs | 7 +- 7 files changed, 84 insertions(+), 66 deletions(-) diff --git a/src/Octoshift/Factories/GithubApiFactory.cs b/src/Octoshift/Factories/GithubApiFactory.cs index 2798ef790..bd33a5ee5 100644 --- a/src/Octoshift/Factories/GithubApiFactory.cs +++ b/src/Octoshift/Factories/GithubApiFactory.cs @@ -30,7 +30,7 @@ GithubApi ISourceGithubApiFactory.Create(string apiUrl, string sourcePersonalAcc apiUrl ??= DEFAULT_API_URL; sourcePersonalAccessToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken(); var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, _retryPolicy, _dateTimeProvider, sourcePersonalAccessToken); - var multipartUploader = new ArchiveUploader(githubClient, _octoLogger, _retryPolicy); + var multipartUploader = new ArchiveUploader(githubClient, _octoLogger, _retryPolicy, _environmentVariableProvider); return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader); } @@ -39,7 +39,7 @@ GithubApi ISourceGithubApiFactory.CreateClientNoSsl(string apiUrl, string source apiUrl ??= DEFAULT_API_URL; sourcePersonalAccessToken ??= _environmentVariableProvider.SourceGithubPersonalAccessToken(); var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("NoSSL"), _versionProvider, _retryPolicy, _dateTimeProvider, sourcePersonalAccessToken); - var multipartUploader = new ArchiveUploader(githubClient, _octoLogger, _retryPolicy); + var multipartUploader = new ArchiveUploader(githubClient, _octoLogger, _retryPolicy, _environmentVariableProvider); return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader); } @@ -48,7 +48,7 @@ GithubApi ITargetGithubApiFactory.Create(string apiUrl, string targetPersonalAcc apiUrl ??= DEFAULT_API_URL; targetPersonalAccessToken ??= _environmentVariableProvider.TargetGithubPersonalAccessToken(); var githubClient = new GithubClient(_octoLogger, _clientFactory.CreateClient("Default"), _versionProvider, _retryPolicy, _dateTimeProvider, targetPersonalAccessToken); - var multipartUploader = new ArchiveUploader(githubClient, _octoLogger, _retryPolicy); + var multipartUploader = new ArchiveUploader(githubClient, _octoLogger, _retryPolicy, _environmentVariableProvider); return new GithubApi(githubClient, apiUrl, _retryPolicy, multipartUploader); } } diff --git a/src/Octoshift/Services/ArchiveUploader.cs b/src/Octoshift/Services/ArchiveUploader.cs index 12369bcd0..462145b18 100644 --- a/src/Octoshift/Services/ArchiveUploader.cs +++ b/src/Octoshift/Services/ArchiveUploader.cs @@ -12,19 +12,22 @@ namespace OctoshiftCLI.Services; public class ArchiveUploader { + private const int MIN_MULTIPART_BYTES = 5 * 1024 * 1024; // 5 MiB minimum size for multipart upload. Don't allow overrides smaller than this. + private readonly GithubClient _client; private readonly OctoLogger _log; + private readonly EnvironmentVariableProvider _environmentVariableProvider; internal int _streamSizeLimit = 100 * 1024 * 1024; // 100 MiB - private const int MIN_MULTIPART_BYTES = 5 * 1024 * 1024; // 5 MiB minimum size for multipart upload. Don't allow overrides smaller than this. private readonly RetryPolicy _retryPolicy; private const string BASE_URL = "https://uploads.github.com"; - public ArchiveUploader(GithubClient client, OctoLogger log, RetryPolicy retryPolicy) + public ArchiveUploader(GithubClient client, OctoLogger log, RetryPolicy retryPolicy, EnvironmentVariableProvider environmentVariableProvider) { _client = client; _log = log; _retryPolicy = retryPolicy; + _environmentVariableProvider = environmentVariableProvider; SetStreamSizeLimitFromEnvironment(); } @@ -166,7 +169,7 @@ private Uri GetNextUrl(IEnumerable>> he private void SetStreamSizeLimitFromEnvironment() { - var envValue = Environment.GetEnvironmentVariable("GITHUB_OWNED_STORAGE_MULTIPART_BYTES"); + var envValue = _environmentVariableProvider.GithubOwnedStorageMultipartBytes(); if (!int.TryParse(envValue, out var limit) || limit <= 0) { return; diff --git a/src/Octoshift/Services/EnvironmentVariableProvider.cs b/src/Octoshift/Services/EnvironmentVariableProvider.cs index b68cbc4b7..61beacd6f 100644 --- a/src/Octoshift/Services/EnvironmentVariableProvider.cs +++ b/src/Octoshift/Services/EnvironmentVariableProvider.cs @@ -18,6 +18,7 @@ public class EnvironmentVariableProvider private const string SMB_PASSWORD = "SMB_PASSWORD"; private const string GEI_SKIP_STATUS_CHECK = "GEI_SKIP_STATUS_CHECK"; private const string GEI_SKIP_VERSION_CHECK = "GEI_SKIP_VERSION_CHECK"; + private const string GITHUB_OWNED_STORAGE_MULTIPART_BYTES = "GITHUB_OWNED_STORAGE_MULTIPART_BYTES"; private readonly OctoLogger _logger; @@ -65,6 +66,9 @@ public virtual string SkipStatusCheck(bool throwIfNotFound = false) => public virtual string SkipVersionCheck(bool throwIfNotFound = false) => GetValue(GEI_SKIP_VERSION_CHECK, throwIfNotFound); + public virtual string GithubOwnedStorageMultipartBytes(bool throwIfNotFound = false) => + GetValue(GITHUB_OWNED_STORAGE_MULTIPART_BYTES, throwIfNotFound); + private string GetValue(string name, bool throwIfNotFound) { var value = Environment.GetEnvironmentVariable(name); diff --git a/src/OctoshiftCLI.IntegrationTests/BbsToGithub.cs b/src/OctoshiftCLI.IntegrationTests/BbsToGithub.cs index fdc7d3ed4..d02bcebb1 100644 --- a/src/OctoshiftCLI.IntegrationTests/BbsToGithub.cs +++ b/src/OctoshiftCLI.IntegrationTests/BbsToGithub.cs @@ -60,7 +60,8 @@ public BbsToGithub(ITestOutputHelper output) _targetGithubHttpClient = new HttpClient(); _targetGithubClient = new GithubClient(_logger, _targetGithubHttpClient, new VersionChecker(_versionClient, _logger), new RetryPolicy(_logger), new DateTimeProvider(), targetGithubToken); var retryPolicy = new RetryPolicy(_logger); - _archiveUploader = new ArchiveUploader(_targetGithubClient, _logger, retryPolicy); + var environmentVariableProvider = new EnvironmentVariableProvider(_logger); + _archiveUploader = new ArchiveUploader(_targetGithubClient, _logger, retryPolicy, environmentVariableProvider); _targetGithubApi = new GithubApi(_targetGithubClient, "https://api.github.com", new RetryPolicy(_logger), _archiveUploader); _blobServiceClient = new BlobServiceClient(_azureStorageConnectionString); diff --git a/src/OctoshiftCLI.IntegrationTests/GhesToGithub.cs b/src/OctoshiftCLI.IntegrationTests/GhesToGithub.cs index cf9fcc314..8c168f97b 100644 --- a/src/OctoshiftCLI.IntegrationTests/GhesToGithub.cs +++ b/src/OctoshiftCLI.IntegrationTests/GhesToGithub.cs @@ -48,10 +48,11 @@ public GhesToGithub(ITestOutputHelper output) _versionClient = new HttpClient(); var retryPolicy = new RetryPolicy(logger); - _archiveUploader = new ArchiveUploader(_targetGithubClient, logger, retryPolicy); + var environmentVariableProvider = new EnvironmentVariableProvider(logger); _sourceGithubHttpClient = new HttpClient(); _sourceGithubClient = new GithubClient(logger, _sourceGithubHttpClient, new VersionChecker(_versionClient, logger), new RetryPolicy(logger), new DateTimeProvider(), sourceGithubToken); + _archiveUploader = new ArchiveUploader(_sourceGithubClient, logger, retryPolicy, environmentVariableProvider); _sourceGithubApi = new GithubApi(_sourceGithubClient, GHES_API_URL, new RetryPolicy(logger), _archiveUploader); _targetGithubHttpClient = new HttpClient(); diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs index df64ee143..b92ef0e59 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs @@ -11,40 +11,20 @@ namespace OctoshiftCLI.Tests.Octoshift.Services; -[Collection("Environment Variables")] -public class ArchiveUploaderTests : IDisposable +public class ArchiveUploaderTests { - private bool _disposed; - private readonly Mock _githubClientMock; private readonly Mock _logMock; + private readonly Mock _environmentVariableProviderMock; private readonly ArchiveUploader _archiveUploader; - private const string ENV_VAR_NAME = "GITHUB_OWNED_STORAGE_MULTIPART_BYTES"; public ArchiveUploaderTests() { _logMock = TestHelpers.CreateMock(); _githubClientMock = TestHelpers.CreateMock(); + _environmentVariableProviderMock = TestHelpers.CreateMock(); var retryPolicy = new RetryPolicy(_logMock.Object) { _httpRetryInterval = 1, _retryInterval = 0 }; - _archiveUploader = new ArchiveUploader(_githubClientMock.Object, _logMock.Object, retryPolicy); - } - - protected virtual void Dispose(bool disposing) - { - if (!_disposed) - { - if (disposing) - { - // Clean up environment variable after each test - Environment.SetEnvironmentVariable(ENV_VAR_NAME, null); - } - _disposed = true; - } - } - public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); + _archiveUploader = new ArchiveUploader(_githubClientMock.Object, _logMock.Object, retryPolicy, _environmentVariableProviderMock.Object); } [Fact] @@ -64,14 +44,17 @@ public void Constructor_Should_Use_Valid_Environment_Variable_Value() { // Arrange var customSize = 10 * 1024 * 1024; // 10 MiB - Environment.SetEnvironmentVariable(ENV_VAR_NAME, customSize.ToString()); - var logMock = TestHelpers.CreateMock(); var githubClientMock = TestHelpers.CreateMock(); + var environmentVariableProviderMock = TestHelpers.CreateMock(); var retryPolicy = new RetryPolicy(logMock.Object); + environmentVariableProviderMock + .Setup(x => x.GithubOwnedStorageMultipartBytes(false)) + .Returns(customSize.ToString()); + // Act - var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy, environmentVariableProviderMock.Object); // Assert archiveUploader._streamSizeLimit.Should().Be(customSize); @@ -82,15 +65,18 @@ public void Constructor_Should_Use_Valid_Environment_Variable_Value() public void Constructor_Should_Use_Default_When_Environment_Variable_Not_Set() { // Arrange - Environment.SetEnvironmentVariable(ENV_VAR_NAME, null); var defaultSize = 100 * 1024 * 1024; // 100 MiB - var logMock = TestHelpers.CreateMock(); var githubClientMock = TestHelpers.CreateMock(); + var environmentVariableProviderMock = TestHelpers.CreateMock(); var retryPolicy = new RetryPolicy(logMock.Object); + environmentVariableProviderMock + .Setup(x => x.GithubOwnedStorageMultipartBytes(false)) + .Returns((string)null); + // Act - var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy, environmentVariableProviderMock.Object); // Assert archiveUploader._streamSizeLimit.Should().Be(defaultSize); @@ -100,15 +86,18 @@ public void Constructor_Should_Use_Default_When_Environment_Variable_Not_Set() public void Constructor_Should_Use_Default_When_Environment_Variable_Is_Invalid() { // Arrange - Environment.SetEnvironmentVariable(ENV_VAR_NAME, "invalid_value"); var defaultSize = 100 * 1024 * 1024; // 100 MiB - var logMock = TestHelpers.CreateMock(); var githubClientMock = TestHelpers.CreateMock(); + var environmentVariableProviderMock = TestHelpers.CreateMock(); var retryPolicy = new RetryPolicy(logMock.Object); + environmentVariableProviderMock + .Setup(x => x.GithubOwnedStorageMultipartBytes(false)) + .Returns("invalid_value"); + // Act - var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy, environmentVariableProviderMock.Object); // Assert archiveUploader._streamSizeLimit.Should().Be(defaultSize); @@ -118,15 +107,18 @@ public void Constructor_Should_Use_Default_When_Environment_Variable_Is_Invalid( public void Constructor_Should_Use_Default_When_Environment_Variable_Is_Zero() { // Arrange - Environment.SetEnvironmentVariable(ENV_VAR_NAME, "0"); var defaultSize = 100 * 1024 * 1024; // 100 MiB - var logMock = TestHelpers.CreateMock(); var githubClientMock = TestHelpers.CreateMock(); + var environmentVariableProviderMock = TestHelpers.CreateMock(); var retryPolicy = new RetryPolicy(logMock.Object); + environmentVariableProviderMock + .Setup(x => x.GithubOwnedStorageMultipartBytes(false)) + .Returns("0"); + // Act - var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy, environmentVariableProviderMock.Object); // Assert archiveUploader._streamSizeLimit.Should().Be(defaultSize); @@ -136,15 +128,18 @@ public void Constructor_Should_Use_Default_When_Environment_Variable_Is_Zero() public void Constructor_Should_Use_Default_When_Environment_Variable_Is_Negative() { // Arrange - Environment.SetEnvironmentVariable(ENV_VAR_NAME, "-1000"); var defaultSize = 100 * 1024 * 1024; // 100 MiB - var logMock = TestHelpers.CreateMock(); var githubClientMock = TestHelpers.CreateMock(); + var environmentVariableProviderMock = TestHelpers.CreateMock(); var retryPolicy = new RetryPolicy(logMock.Object); + environmentVariableProviderMock + .Setup(x => x.GithubOwnedStorageMultipartBytes(false)) + .Returns("-1000"); + // Act - var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy, environmentVariableProviderMock.Object); // Assert archiveUploader._streamSizeLimit.Should().Be(defaultSize); @@ -157,14 +152,17 @@ public void Constructor_Should_Use_Default_And_Log_Warning_When_Environment_Vari var belowMinimumSize = 1024 * 1024; // 1 MiB (below 5 MiB minimum) var defaultSize = 100 * 1024 * 1024; // 100 MiB var minSize = 5 * 1024 * 1024; // 5 MiB minimum - Environment.SetEnvironmentVariable(ENV_VAR_NAME, belowMinimumSize.ToString()); - var logMock = TestHelpers.CreateMock(); var githubClientMock = TestHelpers.CreateMock(); + var environmentVariableProviderMock = TestHelpers.CreateMock(); var retryPolicy = new RetryPolicy(logMock.Object); + environmentVariableProviderMock + .Setup(x => x.GithubOwnedStorageMultipartBytes(false)) + .Returns(belowMinimumSize.ToString()); + // Act - var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy, environmentVariableProviderMock.Object); // Assert archiveUploader._streamSizeLimit.Should().Be(defaultSize); @@ -176,14 +174,17 @@ public void Constructor_Should_Accept_Value_Equal_To_Minimum() { // Arrange var minimumSize = 5 * 1024 * 1024; // 5 MiB minimum - Environment.SetEnvironmentVariable(ENV_VAR_NAME, minimumSize.ToString()); - var logMock = TestHelpers.CreateMock(); var githubClientMock = TestHelpers.CreateMock(); + var environmentVariableProviderMock = TestHelpers.CreateMock(); var retryPolicy = new RetryPolicy(logMock.Object); + environmentVariableProviderMock + .Setup(x => x.GithubOwnedStorageMultipartBytes(false)) + .Returns(minimumSize.ToString()); + // Act - var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy, environmentVariableProviderMock.Object); // Assert archiveUploader._streamSizeLimit.Should().Be(minimumSize); @@ -195,14 +196,17 @@ public void Constructor_Should_Accept_Large_Valid_Value() { // Arrange var largeSize = 500 * 1024 * 1024; // 500 MiB - Environment.SetEnvironmentVariable(ENV_VAR_NAME, largeSize.ToString()); - var logMock = TestHelpers.CreateMock(); var githubClientMock = TestHelpers.CreateMock(); + var environmentVariableProviderMock = TestHelpers.CreateMock(); var retryPolicy = new RetryPolicy(logMock.Object); + environmentVariableProviderMock + .Setup(x => x.GithubOwnedStorageMultipartBytes(false)) + .Returns(largeSize.ToString()); + // Act - var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy); + var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy, environmentVariableProviderMock.Object); // Assert archiveUploader._streamSizeLimit.Should().Be(largeSize); @@ -291,7 +295,7 @@ public async Task Upload_Should_Retry_Failed_Upload_Part_Patch_Requests() // Mocking the initial POST request to initiate multipart upload _githubClientMock .Setup(m => m.PostWithFullResponseAsync($"{baseUrl}{initialUploadUrl}", It.Is(x => x.ToJson() == startUploadBody.ToJson()), null)) - .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [firstUploadUrl]) })); + .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", new[] { firstUploadUrl }) })); // Mocking PATCH requests for each part upload _githubClientMock // first PATCH request @@ -299,11 +303,11 @@ public async Task Upload_Should_Retry_Failed_Upload_Part_Patch_Requests() It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 1, 2 }.ToJson()), null)) .ThrowsAsync(new TimeoutException("The operation was canceled.")) .ThrowsAsync(new TimeoutException("The operation was canceled.")) - .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [secondUploadUrl]) })); + .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", new[] { secondUploadUrl }) })); _githubClientMock // second PATCH request .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{secondUploadUrl}", It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 3 }.ToJson()), null)) - .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [lastUrl]) })); + .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", new[] { lastUrl }) })); // Mocking the final PUT request to complete the multipart upload _githubClientMock @@ -348,18 +352,18 @@ public async Task Upload_Should_Retry_Failed_Start_Upload_Post_Request() .SetupSequence(m => m.PostWithFullResponseAsync($"{baseUrl}{initialUploadUrl}", It.Is(x => x.ToJson() == startUploadBody.ToJson()), null)) .ThrowsAsync(new TimeoutException("The operation was canceled.")) .ThrowsAsync(new TimeoutException("The operation was canceled.")) - .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [firstUploadUrl]) })); + .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", new[] { firstUploadUrl }) })); // Mocking PATCH requests for each part upload _githubClientMock // first PATCH request .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{firstUploadUrl}", It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 1, 2 }.ToJson()), null)) - .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [secondUploadUrl]) })); + .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", new[] { secondUploadUrl }) })); _githubClientMock // second PATCH request .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{secondUploadUrl}", It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 3 }.ToJson()), null)) - .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [lastUrl]) })); + .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", new[] { lastUrl }) })); // Mocking the final PUT request to complete the multipart upload _githubClientMock @@ -402,18 +406,18 @@ public async Task Upload_Should_Retry_Failed_Complete_Upload_Put_Request() // Mocking the initial POST request to initiate multipart upload _githubClientMock .Setup(m => m.PostWithFullResponseAsync($"{baseUrl}{initialUploadUrl}", It.Is(x => x.ToJson() == startUploadBody.ToJson()), null)) - .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [firstUploadUrl]) })); + .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", new[] { firstUploadUrl }) })); // Mocking PATCH requests for each part upload _githubClientMock // first PATCH request .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{firstUploadUrl}", It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 1, 2 }.ToJson()), null)) - .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [secondUploadUrl]) })); + .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", new[] { secondUploadUrl }) })); _githubClientMock // second PATCH request .Setup(m => m.PatchWithFullResponseAsync($"{baseUrl}{secondUploadUrl}", It.Is(x => x.ReadAsByteArrayAsync().Result.ToJson() == new byte[] { 3 }.ToJson()), null)) - .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", [lastUrl]) })); + .ReturnsAsync((It.IsAny(), new[] { new KeyValuePair>("Location", new[] { lastUrl }) })); // Mocking the final PUT request to complete the multipart upload _githubClientMock diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs index f1027f1ac..af25c0dd0 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs @@ -19,6 +19,7 @@ public class GithubApiTests { private const string API_URL = "https://api.github.com"; private readonly RetryPolicy _retryPolicy = new(TestHelpers.CreateMock().Object) { _httpRetryInterval = 0, _retryInterval = 0 }; + private readonly Mock _logMock = TestHelpers.CreateMock(); private readonly Mock _githubClientMock = TestHelpers.CreateMock(); private readonly Mock _archiveUploader; @@ -46,7 +47,11 @@ public class GithubApiTests public GithubApiTests() { - _archiveUploader = TestHelpers.CreateMock(); + _archiveUploader = new Mock( + _githubClientMock.Object, + _logMock.Object, + _retryPolicy, + TestHelpers.CreateMock().Object); _githubApi = new GithubApi(_githubClientMock.Object, API_URL, _retryPolicy, _archiveUploader.Object); } From 688d6904a362b7885c178a46524a5ce7aec968db Mon Sep 17 00:00:00 2001 From: Tiago Pascoal Date: Wed, 30 Jul 2025 23:50:18 +0100 Subject: [PATCH 6/6] Fix code scanning warning --- .../Octoshift/Services/ArchiveUploadersTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs index b92ef0e59..3d2b7566e 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/ArchiveUploadersTests.cs @@ -73,7 +73,7 @@ public void Constructor_Should_Use_Default_When_Environment_Variable_Not_Set() environmentVariableProviderMock .Setup(x => x.GithubOwnedStorageMultipartBytes(false)) - .Returns((string)null); + .Returns(() => null); // Act var archiveUploader = new ArchiveUploader(githubClientMock.Object, logMock.Object, retryPolicy, environmentVariableProviderMock.Object);