From 71e329a83ad5c86239d465cf2fc903be7d622c82 Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Tue, 21 Jan 2025 11:18:51 +0100 Subject: [PATCH 1/8] Move Current.getStatusCount to BuildSystemClient.getStatusCount --- Sources/App/Commands/TriggerBuilds.swift | 6 +- Sources/App/Core/AppEnvironment.swift | 8 - .../Core/Dependencies/BuildSystemClient.swift | 54 +++++++ Tests/AppTests/BuildTriggerTests.swift | 145 ++++++++++-------- .../AppTests/Mocks/AppEnvironment+mock.swift | 1 - 5 files changed, 137 insertions(+), 77 deletions(-) create mode 100644 Sources/App/Core/Dependencies/BuildSystemClient.swift diff --git a/Sources/App/Commands/TriggerBuilds.swift b/Sources/App/Commands/TriggerBuilds.swift index 119b13ec5..6cd45c322 100644 --- a/Sources/App/Commands/TriggerBuilds.swift +++ b/Sources/App/Commands/TriggerBuilds.swift @@ -179,6 +179,7 @@ func triggerBuilds(on database: Database, packages: [Package.Id], force: Bool = false) async throws { @Dependency(\.environment) var environment + @Dependency(\.buildSystem) var buildSystem guard environment.allowBuildTriggers() else { Current.logger().info("Build trigger override switch OFF - no builds are being triggered") @@ -196,8 +197,9 @@ func triggerBuilds(on database: Database, } } - async let pendingJobsTask = Current.getStatusCount(client, .pending) - async let runningJobsTask = Current.getStatusCount(client, .running) + let getStatusCount = buildSystem.getStatusCount + async let pendingJobsTask = getStatusCount(client, .pending) + async let runningJobsTask = getStatusCount(client, .running) let pendingJobs = try await pendingJobsTask let runningJobs = try await runningJobsTask diff --git a/Sources/App/Core/AppEnvironment.swift b/Sources/App/Core/AppEnvironment.swift index 4dd628ba6..66fd114a5 100644 --- a/Sources/App/Core/AppEnvironment.swift +++ b/Sources/App/Core/AppEnvironment.swift @@ -24,7 +24,6 @@ import FoundationNetworking struct AppEnvironment: Sendable { var fileManager: FileManager - var getStatusCount: @Sendable (_ client: Client, _ status: Gitlab.Builder.Status) async throws -> Int var git: Git var gitlabApiToken: @Sendable () -> String? var gitlabPipelineToken: @Sendable () -> String? @@ -48,13 +47,6 @@ extension AppEnvironment { static let live = AppEnvironment( fileManager: .live, - getStatusCount: { client, status in - try await Gitlab.Builder.getStatusCount(client: client, - status: status, - page: 1, - pageSize: 100, - maxPageCount: 5) - }, git: .live, gitlabApiToken: { Environment.get("GITLAB_API_TOKEN") }, gitlabPipelineToken: { Environment.get("GITLAB_PIPELINE_TOKEN") }, diff --git a/Sources/App/Core/Dependencies/BuildSystemClient.swift b/Sources/App/Core/Dependencies/BuildSystemClient.swift new file mode 100644 index 000000000..4a564ef1b --- /dev/null +++ b/Sources/App/Core/Dependencies/BuildSystemClient.swift @@ -0,0 +1,54 @@ +// Copyright Dave Verwer, Sven A. Schmidt, and other contributors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import Dependencies +import DependenciesMacros +import Vapor + + +@DependencyClient +struct BuildSystemClient { +#warning("remove client") + var getStatusCount: @Sendable (_ client: Client, _ status: Gitlab.Builder.Status) async throws -> Int +} + + +extension BuildSystemClient: DependencyKey { + static var liveValue: Self { + .init( + getStatusCount: { client, status in + try await Gitlab.Builder.getStatusCount(client: client, + status: status, + page: 1, + pageSize: 100, + maxPageCount: 5) + } + ) + } +} + + +extension BuildSystemClient: TestDependencyKey { + static var testValue: Self { Self() } +} + + +extension DependencyValues { + var buildSystem: BuildSystemClient { + get { self[BuildSystemClient.self] } + set { self[BuildSystemClient.self] = newValue } + } +} + + diff --git a/Tests/AppTests/BuildTriggerTests.swift b/Tests/AppTests/BuildTriggerTests.swift index a30cde1e0..7e5d2cad0 100644 --- a/Tests/AppTests/BuildTriggerTests.swift +++ b/Tests/AppTests/BuildTriggerTests.swift @@ -600,88 +600,98 @@ class BuildTriggerTests: AppTestCase { } do { // fist run: we are at capacity and should not be triggering more builds - Current.getStatusCount = { _, _ in 300 } - - let pkgId = UUID() - let versionId = UUID() - let p = Package(id: pkgId, url: "1") - try await p.save(on: app.db) - try await Version(id: versionId, package: p, latest: .defaultBranch, reference: .branch("main")) - .save(on: app.db) - - // MUT - try await triggerBuilds(on: app.db, - client: client, - mode: .packageId(pkgId, force: false)) + try await withDependencies { + $0.buildSystem.getStatusCount = { @Sendable _, _ in 300 } + } operation: { + let pkgId = UUID() + let versionId = UUID() + let p = Package(id: pkgId, url: "1") + try await p.save(on: app.db) + try await Version(id: versionId, package: p, latest: .defaultBranch, reference: .branch("main")) + .save(on: app.db) - // validate - XCTAssertEqual(triggerCount, 0) - // ensure no build stubs have been created either - let v = try await Version.find(versionId, on: app.db) - try await v?.$builds.load(on: app.db) - XCTAssertEqual(v?.builds.count, 0) + // MUT + try await triggerBuilds(on: app.db, + client: client, + mode: .packageId(pkgId, force: false)) + + // validate + XCTAssertEqual(triggerCount, 0) + // ensure no build stubs have been created either + let v = try await Version.find(versionId, on: app.db) + try await v?.$builds.load(on: app.db) + XCTAssertEqual(v?.builds.count, 0) + } } triggerCount = 0 do { // second run: we are just below capacity and allow more builds to be triggered - Current.getStatusCount = { c, _ in 299 } - - let pkgId = UUID() - let versionId = UUID() - let p = Package(id: pkgId, url: "2") - try await p.save(on: app.db) - try await Version(id: versionId, package: p, latest: .defaultBranch, reference: .branch("main")) - .save(on: app.db) - - // MUT - try await triggerBuilds(on: app.db, - client: client, - mode: .packageId(pkgId, force: false)) + try await withDependencies { + $0.buildSystem.getStatusCount = { @Sendable _, _ in 299 } + } operation: { + let pkgId = UUID() + let versionId = UUID() + let p = Package(id: pkgId, url: "2") + try await p.save(on: app.db) + try await Version(id: versionId, package: p, latest: .defaultBranch, reference: .branch("main")) + .save(on: app.db) - // validate - XCTAssertEqual(triggerCount, 27) - // ensure builds are now in progress - let v = try await Version.find(versionId, on: app.db) - try await v?.$builds.load(on: app.db) - XCTAssertEqual(v?.builds.count, 27) + // MUT + try await triggerBuilds(on: app.db, + client: client, + mode: .packageId(pkgId, force: false)) + + // validate + XCTAssertEqual(triggerCount, 27) + // ensure builds are now in progress + let v = try await Version.find(versionId, on: app.db) + try await v?.$builds.load(on: app.db) + XCTAssertEqual(v?.builds.count, 27) + } } do { // third run: we are at capacity and using the `force` flag - Current.getStatusCount = { c, _ in 300 } - - var triggerCount = 0 - let client = MockClient { _, res in - triggerCount += 1 - try? res.content.encode( - Gitlab.Builder.Response.init(webUrl: "http://web_url") - ) - } - - let pkgId = UUID() - let versionId = UUID() - let p = Package(id: pkgId, url: "3") - try await p.save(on: app.db) - try await Version(id: versionId, package: p, latest: .defaultBranch, reference: .branch("main")) - .save(on: app.db) + try await withDependencies { + $0.buildSystem.getStatusCount = { @Sendable _, _ in 300 } + } operation: { + var triggerCount = 0 + let client = MockClient { _, res in + triggerCount += 1 + try? res.content.encode( + Gitlab.Builder.Response.init(webUrl: "http://web_url") + ) + } - // MUT - try await triggerBuilds(on: app.db, - client: client, - mode: .packageId(pkgId, force: true)) + let pkgId = UUID() + let versionId = UUID() + let p = Package(id: pkgId, url: "3") + try await p.save(on: app.db) + try await Version(id: versionId, package: p, latest: .defaultBranch, reference: .branch("main")) + .save(on: app.db) - // validate - XCTAssertEqual(triggerCount, 27) - // ensure builds are now in progress - let v = try await Version.find(versionId, on: app.db) - try await v?.$builds.load(on: app.db) - XCTAssertEqual(v?.builds.count, 27) + // MUT + try await triggerBuilds(on: app.db, + client: client, + mode: .packageId(pkgId, force: true)) + + // validate + XCTAssertEqual(triggerCount, 27) + // ensure builds are now in progress + let v = try await Version.find(versionId, on: app.db) + try await v?.$builds.load(on: app.db) + XCTAssertEqual(v?.builds.count, 27) + } } } } func test_triggerBuilds_multiplePackages() async throws { + let triggerCount = NIOLockedValueBox(0) try await withDependencies { + $0.buildSystem.getStatusCount = { @Sendable c, _ in + 299 + triggerCount.withLockedValue { $0 } + } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } @@ -708,14 +718,12 @@ class BuildTriggerTests: AppTestCase { swiftVersion: swiftVersion, versionID: versionID) } - let triggerCount = NIOLockedValueBox(0) let client = MockClient { _, res in triggerCount.withLockedValue { $0 += 1 } try? res.content.encode( Gitlab.Builder.Response.init(webUrl: "http://web_url") ) } - Current.getStatusCount = { c, _ in 299 + triggerCount.withLockedValue { $0 } } let pkgIds = [UUID(), UUID()] for id in pkgIds { @@ -737,6 +745,7 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuilds_trimming() async throws { try await withDependencies { + $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } @@ -776,6 +785,7 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuilds_error() async throws { try await withDependencies { + $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } @@ -905,6 +915,7 @@ class BuildTriggerTests: AppTestCase { func test_override_switch() async throws { try await withDependencies { + $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } @@ -982,6 +993,7 @@ class BuildTriggerTests: AppTestCase { func test_downscaling() async throws { try await withDependencies { + $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } @@ -1059,6 +1071,7 @@ class BuildTriggerTests: AppTestCase { func test_downscaling_allow_list_override() async throws { try await withDependencies { + $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } diff --git a/Tests/AppTests/Mocks/AppEnvironment+mock.swift b/Tests/AppTests/Mocks/AppEnvironment+mock.swift index e17d4fdc2..9cf0d69b9 100644 --- a/Tests/AppTests/Mocks/AppEnvironment+mock.swift +++ b/Tests/AppTests/Mocks/AppEnvironment+mock.swift @@ -23,7 +23,6 @@ extension AppEnvironment { static func mock(eventLoop: EventLoop) -> Self { .init( fileManager: .mock, - getStatusCount: { _, _ in 100 }, git: .mock, gitlabApiToken: { nil }, gitlabPipelineToken: { nil }, From f5733bfe6c1666b2f65529a80f9bcbcd0862ad99 Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Tue, 21 Jan 2025 11:25:56 +0100 Subject: [PATCH 2/8] Move Current.gitlabApiToken to buildSystem.gitlabApiToken --- Sources/App/Core/AppEnvironment.swift | 2 - .../Core/Dependencies/BuildSystemClient.swift | 2 + Sources/App/Core/Gitlab.swift | 3 +- Tests/AppTests/GitlabBuilderTests.swift | 51 ++++++++++--------- .../AppTests/Mocks/AppEnvironment+mock.swift | 1 - 5 files changed, 31 insertions(+), 28 deletions(-) diff --git a/Sources/App/Core/AppEnvironment.swift b/Sources/App/Core/AppEnvironment.swift index 66fd114a5..64d6a6b2d 100644 --- a/Sources/App/Core/AppEnvironment.swift +++ b/Sources/App/Core/AppEnvironment.swift @@ -25,7 +25,6 @@ import FoundationNetworking struct AppEnvironment: Sendable { var fileManager: FileManager var git: Git - var gitlabApiToken: @Sendable () -> String? var gitlabPipelineToken: @Sendable () -> String? var gitlabPipelineLimit: @Sendable () -> Int var logger: @Sendable () -> Logger @@ -48,7 +47,6 @@ extension AppEnvironment { static let live = AppEnvironment( fileManager: .live, git: .live, - gitlabApiToken: { Environment.get("GITLAB_API_TOKEN") }, gitlabPipelineToken: { Environment.get("GITLAB_PIPELINE_TOKEN") }, gitlabPipelineLimit: { Environment.get("GITLAB_PIPELINE_LIMIT").flatMap(Int.init) diff --git a/Sources/App/Core/Dependencies/BuildSystemClient.swift b/Sources/App/Core/Dependencies/BuildSystemClient.swift index 4a564ef1b..c3a5fde90 100644 --- a/Sources/App/Core/Dependencies/BuildSystemClient.swift +++ b/Sources/App/Core/Dependencies/BuildSystemClient.swift @@ -19,6 +19,7 @@ import Vapor @DependencyClient struct BuildSystemClient { + var gitlabApiToken: @Sendable () -> String? #warning("remove client") var getStatusCount: @Sendable (_ client: Client, _ status: Gitlab.Builder.Status) async throws -> Int } @@ -27,6 +28,7 @@ struct BuildSystemClient { extension BuildSystemClient: DependencyKey { static var liveValue: Self { .init( + gitlabApiToken: { Environment.get("GITLAB_API_TOKEN") }, getStatusCount: { client, status in try await Gitlab.Builder.getStatusCount(client: client, status: status, diff --git a/Sources/App/Core/Gitlab.swift b/Sources/App/Core/Gitlab.swift index 2f83402d6..0e53e45e1 100644 --- a/Sources/App/Core/Gitlab.swift +++ b/Sources/App/Core/Gitlab.swift @@ -157,7 +157,8 @@ extension Gitlab.Builder { status: Status, page: Int, pageSize: Int = 20) async throws -> [Pipeline] { - guard let apiToken = Current.gitlabApiToken() else { throw Gitlab.Error.missingToken } + @Dependency(\.buildSystem) var buildSystem + guard let apiToken = buildSystem.gitlabApiToken() else { throw Gitlab.Error.missingToken } let uri: URI = .init(string: "\(projectURL)/pipelines?status=\(status)&page=\(page)&per_page=\(pageSize)") let response = try await client.get(uri, headers: HTTPHeaders([("Authorization", "Bearer \(apiToken)")])) diff --git a/Tests/AppTests/GitlabBuilderTests.swift b/Tests/AppTests/GitlabBuilderTests.swift index 2884dbcf8..a7d19eee4 100644 --- a/Tests/AppTests/GitlabBuilderTests.swift +++ b/Tests/AppTests/GitlabBuilderTests.swift @@ -137,32 +137,35 @@ class GitlabBuilderTests: AppTestCase { } func test_getStatusCount() async throws { - Current.gitlabApiToken = { "api token" } - Current.gitlabPipelineToken = { nil } - - var page = 1 - let client = MockClient { req, res in - XCTAssertEqual(req.url.string, "https://gitlab.com/api/v4/projects/19564054/pipelines?status=pending&page=\(page)&per_page=20") - res.status = .ok - let pending = #"{"id": 1, "status": "pending"}"# - switch page { - case 1: - let list = Array(repeating: pending, count: 20).joined(separator: ", ") - res.body = makeBody("[\(list)]") - case 2: - let list = Array(repeating: pending, count: 10).joined(separator: ", ") - res.body = makeBody("[\(list)]") - default: - XCTFail("unexpected page: \(page)") + try await withDependencies { + $0.buildSystem.gitlabApiToken = { "api token" } + } operation: { + Current.gitlabPipelineToken = { nil } + + var page = 1 + let client = MockClient { req, res in + XCTAssertEqual(req.url.string, "https://gitlab.com/api/v4/projects/19564054/pipelines?status=pending&page=\(page)&per_page=20") + res.status = .ok + let pending = #"{"id": 1, "status": "pending"}"# + switch page { + case 1: + let list = Array(repeating: pending, count: 20).joined(separator: ", ") + res.body = makeBody("[\(list)]") + case 2: + let list = Array(repeating: pending, count: 10).joined(separator: ", ") + res.body = makeBody("[\(list)]") + default: + XCTFail("unexpected page: \(page)") + } + page += 1 } - page += 1 + + let res = try await Gitlab.Builder.getStatusCount(client: client, + status: .pending, + pageSize: 20, + maxPageCount: 3) + XCTAssertEqual(res, 30) } - - let res = try await Gitlab.Builder.getStatusCount(client: client, - status: .pending, - pageSize: 20, - maxPageCount: 3) - XCTAssertEqual(res, 30) } } diff --git a/Tests/AppTests/Mocks/AppEnvironment+mock.swift b/Tests/AppTests/Mocks/AppEnvironment+mock.swift index 9cf0d69b9..7efaf8206 100644 --- a/Tests/AppTests/Mocks/AppEnvironment+mock.swift +++ b/Tests/AppTests/Mocks/AppEnvironment+mock.swift @@ -24,7 +24,6 @@ extension AppEnvironment { .init( fileManager: .mock, git: .mock, - gitlabApiToken: { nil }, gitlabPipelineToken: { nil }, gitlabPipelineLimit: { Constants.defaultGitlabPipelineLimit }, logger: { logger }, From 2ac40f4d406f053944188d260c8ec8c86abd1761 Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Tue, 21 Jan 2025 11:36:32 +0100 Subject: [PATCH 3/8] Refactor --- .../App/Core/Dependencies/BuildSystemClient.swift | 6 ++++-- Sources/App/Core/Gitlab.swift | 13 ++++++++++--- Tests/AppTests/GitlabBuilderTests.swift | 6 +++--- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/Sources/App/Core/Dependencies/BuildSystemClient.swift b/Sources/App/Core/Dependencies/BuildSystemClient.swift index c3a5fde90..bb3d606e3 100644 --- a/Sources/App/Core/Dependencies/BuildSystemClient.swift +++ b/Sources/App/Core/Dependencies/BuildSystemClient.swift @@ -19,7 +19,7 @@ import Vapor @DependencyClient struct BuildSystemClient { - var gitlabApiToken: @Sendable () -> String? + var apiToken: @Sendable () throws -> String #warning("remove client") var getStatusCount: @Sendable (_ client: Client, _ status: Gitlab.Builder.Status) async throws -> Int } @@ -28,7 +28,9 @@ struct BuildSystemClient { extension BuildSystemClient: DependencyKey { static var liveValue: Self { .init( - gitlabApiToken: { Environment.get("GITLAB_API_TOKEN") }, + apiToken: { + try Environment.get("GITLAB_API_TOKEN").unwrap(or: Gitlab.Error.missingToken) + }, getStatusCount: { client, status in try await Gitlab.Builder.getStatusCount(client: client, status: status, diff --git a/Sources/App/Core/Gitlab.swift b/Sources/App/Core/Gitlab.swift index 0e53e45e1..18dd31196 100644 --- a/Sources/App/Core/Gitlab.swift +++ b/Sources/App/Core/Gitlab.swift @@ -158,10 +158,8 @@ extension Gitlab.Builder { page: Int, pageSize: Int = 20) async throws -> [Pipeline] { @Dependency(\.buildSystem) var buildSystem - guard let apiToken = buildSystem.gitlabApiToken() else { throw Gitlab.Error.missingToken } - let uri: URI = .init(string: "\(projectURL)/pipelines?status=\(status)&page=\(page)&per_page=\(pageSize)") - let response = try await client.get(uri, headers: HTTPHeaders([("Authorization", "Bearer \(apiToken)")])) + let response = try await client.get(uri, headers: .bearer(buildSystem.apiToken())) guard response.status == .ok else { throw Gitlab.Error.requestFailed(response.status, uri) } @@ -198,3 +196,12 @@ private extension DateFormatter { return formatter } } + + +private extension HTTPHeaders { + static func bearer(_ token: String) -> Self { + .init([("Authorization", "Bearer \(token)")]) + } +} + + diff --git a/Tests/AppTests/GitlabBuilderTests.swift b/Tests/AppTests/GitlabBuilderTests.swift index a7d19eee4..31adce90b 100644 --- a/Tests/AppTests/GitlabBuilderTests.swift +++ b/Tests/AppTests/GitlabBuilderTests.swift @@ -138,10 +138,10 @@ class GitlabBuilderTests: AppTestCase { func test_getStatusCount() async throws { try await withDependencies { - $0.buildSystem.gitlabApiToken = { "api token" } + $0.buildSystem.apiToken = { "api token" } } operation: { Current.gitlabPipelineToken = { nil } - + var page = 1 let client = MockClient { req, res in XCTAssertEqual(req.url.string, "https://gitlab.com/api/v4/projects/19564054/pipelines?status=pending&page=\(page)&per_page=20") @@ -159,7 +159,7 @@ class GitlabBuilderTests: AppTestCase { } page += 1 } - + let res = try await Gitlab.Builder.getStatusCount(client: client, status: .pending, pageSize: 20, From a21daa7f2cb60f2c3cf14ef3df4bfdec3fb5d120 Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Tue, 21 Jan 2025 11:47:57 +0100 Subject: [PATCH 4/8] Move Current.gitlabPipelineToken to buildSystem.gitlabPipelineToken --- Sources/App/Core/AppEnvironment.swift | 2 -- .../Core/Dependencies/BuildSystemClient.swift | 4 +++- Sources/App/Core/Gitlab.swift | 3 ++- Tests/AppTests/BuildTests.swift | 4 ++-- Tests/AppTests/BuildTriggerTests.swift | 23 ++++++++----------- Tests/AppTests/GitlabBuilderTests.swift | 19 +++++++-------- Tests/AppTests/MetricsTests.swift | 2 +- .../AppTests/Mocks/AppEnvironment+mock.swift | 1 - 8 files changed, 26 insertions(+), 32 deletions(-) diff --git a/Sources/App/Core/AppEnvironment.swift b/Sources/App/Core/AppEnvironment.swift index 64d6a6b2d..a8941900f 100644 --- a/Sources/App/Core/AppEnvironment.swift +++ b/Sources/App/Core/AppEnvironment.swift @@ -25,7 +25,6 @@ import FoundationNetworking struct AppEnvironment: Sendable { var fileManager: FileManager var git: Git - var gitlabPipelineToken: @Sendable () -> String? var gitlabPipelineLimit: @Sendable () -> Int var logger: @Sendable () -> Logger var setLogger: @Sendable (Logger) -> Void @@ -47,7 +46,6 @@ extension AppEnvironment { static let live = AppEnvironment( fileManager: .live, git: .live, - gitlabPipelineToken: { Environment.get("GITLAB_PIPELINE_TOKEN") }, gitlabPipelineLimit: { Environment.get("GITLAB_PIPELINE_LIMIT").flatMap(Int.init) ?? Constants.defaultGitlabPipelineLimit diff --git a/Sources/App/Core/Dependencies/BuildSystemClient.swift b/Sources/App/Core/Dependencies/BuildSystemClient.swift index bb3d606e3..d44212cb5 100644 --- a/Sources/App/Core/Dependencies/BuildSystemClient.swift +++ b/Sources/App/Core/Dependencies/BuildSystemClient.swift @@ -22,6 +22,7 @@ struct BuildSystemClient { var apiToken: @Sendable () throws -> String #warning("remove client") var getStatusCount: @Sendable (_ client: Client, _ status: Gitlab.Builder.Status) async throws -> Int + var gitlabPipelineToken: @Sendable () -> String? } @@ -37,7 +38,8 @@ extension BuildSystemClient: DependencyKey { page: 1, pageSize: 100, maxPageCount: 5) - } + }, + gitlabPipelineToken: { Environment.get("GITLAB_PIPELINE_TOKEN") } ) } } diff --git a/Sources/App/Core/Gitlab.swift b/Sources/App/Core/Gitlab.swift index 18dd31196..d101da034 100644 --- a/Sources/App/Core/Gitlab.swift +++ b/Sources/App/Core/Gitlab.swift @@ -78,8 +78,9 @@ extension Gitlab.Builder { swiftVersion: SwiftVersion, versionID: Version.Id) async throws -> Build.TriggerResponse { @Dependency(\.environment) var environment + @Dependency(\.buildSystem) var buildSystem - guard let pipelineToken = Current.gitlabPipelineToken(), + guard let pipelineToken = buildSystem.gitlabPipelineToken(), let builderToken = environment.builderToken() else { throw Gitlab.Error.missingToken } guard let awsDocsBucket = environment.awsDocsBucket() else { diff --git a/Tests/AppTests/BuildTests.swift b/Tests/AppTests/BuildTests.swift index 046340e6c..9363ae387 100644 --- a/Tests/AppTests/BuildTests.swift +++ b/Tests/AppTests/BuildTests.swift @@ -130,12 +130,12 @@ class BuildTests: AppTestCase { func test_trigger() async throws { try await withDependencies { + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } $0.environment.siteURL = { "http://example.com" } } operation: { - Current.gitlabPipelineToken = { "pipeline token" } // setup let p = try await savePackage(on: app.db, "1") let v = try Version(package: p, reference: .branch("main")) @@ -198,6 +198,7 @@ class BuildTests: AppTestCase { func test_trigger_isDocBuild() async throws { try await withDependencies { + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } @@ -205,7 +206,6 @@ class BuildTests: AppTestCase { } operation: { // Same test as test_trigger above, except we trigger with isDocBuild: true // and expect a 15m TIMEOUT instead of 10m - Current.gitlabPipelineToken = { "pipeline token" } // setup let p = try await savePackage(on: app.db, "1") let v = try Version(package: p, reference: .branch("main")) diff --git a/Tests/AppTests/BuildTriggerTests.swift b/Tests/AppTests/BuildTriggerTests.swift index 7e5d2cad0..304868980 100644 --- a/Tests/AppTests/BuildTriggerTests.swift +++ b/Tests/AppTests/BuildTriggerTests.swift @@ -342,14 +342,13 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuildsUnchecked() async throws { try await withDependencies { + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } $0.environment.siteURL = { "http://example.com" } } operation: { // setup - Current.gitlabPipelineToken = { "pipeline token" } - // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in @@ -404,6 +403,7 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuildsUnchecked_supported() async throws { try await withDependencies { + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } @@ -412,8 +412,6 @@ class BuildTriggerTests: AppTestCase { } operation: { // Explicitly test the full range of all currently triggered platforms and swift versions // setup - Current.gitlabPipelineToken = { "pipeline token" } - // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in @@ -486,6 +484,7 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuildsUnchecked_build_exists() async throws { try await withDependencies { + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } @@ -502,8 +501,6 @@ class BuildTriggerTests: AppTestCase { // being completely ignored because the command errors out. // See https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/issues/2237 // setup - Current.gitlabPipelineToken = { "pipeline token" } - // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in @@ -566,6 +563,7 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuilds_checked() async throws { try await withDependencies { + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } @@ -577,7 +575,6 @@ class BuildTriggerTests: AppTestCase { } operation: { // Ensure we respect the pipeline limit when triggering builds // setup - Current.gitlabPipelineToken = { "pipeline token" } Current.gitlabPipelineLimit = { 300 } // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request @@ -692,6 +689,7 @@ class BuildTriggerTests: AppTestCase { $0.buildSystem.getStatusCount = { @Sendable c, _ in 299 + triggerCount.withLockedValue { $0 } } + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } @@ -704,7 +702,6 @@ class BuildTriggerTests: AppTestCase { } operation: { // Ensure we respect the pipeline limit when triggering builds for multiple package ids // setup - Current.gitlabPipelineToken = { "pipeline token" } Current.gitlabPipelineLimit = { 300 } // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request @@ -746,6 +743,7 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuilds_trimming() async throws { try await withDependencies { $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } @@ -756,7 +754,6 @@ class BuildTriggerTests: AppTestCase { } operation: { // Ensure we trim builds as part of triggering // setup - Current.gitlabPipelineToken = { "pipeline token" } Current.gitlabPipelineLimit = { 300 } let client = MockClient { _, _ in } @@ -786,6 +783,7 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuilds_error() async throws { try await withDependencies { $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } @@ -797,7 +795,6 @@ class BuildTriggerTests: AppTestCase { } operation: { // Ensure we trim builds as part of triggering // setup - Current.gitlabPipelineToken = { "pipeline token" } Current.gitlabPipelineLimit = { 300 } // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request @@ -916,6 +913,7 @@ class BuildTriggerTests: AppTestCase { func test_override_switch() async throws { try await withDependencies { $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } @@ -926,7 +924,6 @@ class BuildTriggerTests: AppTestCase { } operation: { // Ensure don't trigger if the override is off // setup - Current.gitlabPipelineToken = { "pipeline token" } // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in @@ -994,6 +991,7 @@ class BuildTriggerTests: AppTestCase { func test_downscaling() async throws { try await withDependencies { $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } @@ -1004,7 +1002,6 @@ class BuildTriggerTests: AppTestCase { } operation: { // Test build trigger downscaling behaviour // setup - Current.gitlabPipelineToken = { "pipeline token" } // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in @@ -1072,6 +1069,7 @@ class BuildTriggerTests: AppTestCase { func test_downscaling_allow_list_override() async throws { try await withDependencies { $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } @@ -1082,7 +1080,6 @@ class BuildTriggerTests: AppTestCase { } operation: { // Test build trigger downscaling behaviour for allow-listed packages // setup - Current.gitlabPipelineToken = { "pipeline token" } // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in diff --git a/Tests/AppTests/GitlabBuilderTests.swift b/Tests/AppTests/GitlabBuilderTests.swift index 31adce90b..72fb2a02d 100644 --- a/Tests/AppTests/GitlabBuilderTests.swift +++ b/Tests/AppTests/GitlabBuilderTests.swift @@ -55,12 +55,12 @@ class GitlabBuilderTests: AppTestCase { func test_triggerBuild() async throws { try await withDependencies { + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "docs-bucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } $0.environment.siteURL = { "http://example.com" } } operation: { - Current.gitlabPipelineToken = { "pipeline token" } let buildId = UUID() let versionID = UUID() @@ -104,13 +104,12 @@ class GitlabBuilderTests: AppTestCase { func test_issue_588() async throws { try await withDependencies { + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "docs-bucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } $0.environment.siteURL = { "http://example.com" } } operation: { - Current.gitlabPipelineToken = { "pipeline token" } - var called = false let client = MockClient { req, res in called = true @@ -138,10 +137,9 @@ class GitlabBuilderTests: AppTestCase { func test_getStatusCount() async throws { try await withDependencies { + $0.buildSystem.gitlabPipelineToken = { nil } $0.buildSystem.apiToken = { "api token" } } operation: { - Current.gitlabPipelineToken = { nil } - var page = 1 let client = MockClient { req, res in XCTAssertEqual(req.url.string, "https://gitlab.com/api/v4/projects/19564054/pipelines?status=pending&page=\(page)&per_page=20") @@ -180,6 +178,11 @@ class LiveGitlabBuilderTests: AppTestCase { ) try await withDependencies { + // make sure environment variables are configured for live access + $0.buildSystem.gitlabPipelineToken = { + // This Gitlab token is required in order to trigger the pipeline + ProcessInfo.processInfo.environment["LIVE_GITLAB_PIPELINE_TOKEN"] + } $0.environment.builderToken = { // Set this to a valid value if you want to report build results back to the server ProcessInfo.processInfo.environment["LIVE_BUILDER_TOKEN"] @@ -191,12 +194,6 @@ class LiveGitlabBuilderTests: AppTestCase { // set build branch to trigger on Gitlab.Builder.branch = "main" - // make sure environment variables are configured for live access - Current.gitlabPipelineToken = { - // This Gitlab token is required in order to trigger the pipeline - ProcessInfo.processInfo.environment["LIVE_GITLAB_PIPELINE_TOKEN"] - } - let buildId = UUID() // use a valid uuid from a live db if reporting back should succeed diff --git a/Tests/AppTests/MetricsTests.swift b/Tests/AppTests/MetricsTests.swift index bef5f18b6..1ff0c4faf 100644 --- a/Tests/AppTests/MetricsTests.swift +++ b/Tests/AppTests/MetricsTests.swift @@ -23,10 +23,10 @@ class MetricsTests: AppTestCase { func test_basic() async throws { try await withDependencies { + $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.builderToken = { "builder token" } } operation: { // setup - trigger build to increment counter - Current.gitlabPipelineToken = { "pipeline token" } let versionId = UUID() do { // save minimal package + version let p = Package(id: UUID(), url: "1") diff --git a/Tests/AppTests/Mocks/AppEnvironment+mock.swift b/Tests/AppTests/Mocks/AppEnvironment+mock.swift index 7efaf8206..25470ed42 100644 --- a/Tests/AppTests/Mocks/AppEnvironment+mock.swift +++ b/Tests/AppTests/Mocks/AppEnvironment+mock.swift @@ -24,7 +24,6 @@ extension AppEnvironment { .init( fileManager: .mock, git: .mock, - gitlabPipelineToken: { nil }, gitlabPipelineLimit: { Constants.defaultGitlabPipelineLimit }, logger: { logger }, setLogger: { logger in Self.logger = logger }, From 477cecd52317dc3144bf4f875151ef675a0c987f Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Tue, 21 Jan 2025 11:55:36 +0100 Subject: [PATCH 5/8] Move env variables back to EnvironmentClient --- .../Core/Dependencies/BuildSystemClient.swift | 8 +------- .../Core/Dependencies/EnvironmentClient.swift | 4 ++++ Sources/App/Core/Gitlab.swift | 10 ++++++---- Tests/AppTests/BuildTests.swift | 4 ++-- Tests/AppTests/BuildTriggerTests.swift | 20 +++++++++---------- Tests/AppTests/GitlabBuilderTests.swift | 19 +++++++++--------- Tests/AppTests/MetricsTests.swift | 2 +- 7 files changed, 33 insertions(+), 34 deletions(-) diff --git a/Sources/App/Core/Dependencies/BuildSystemClient.swift b/Sources/App/Core/Dependencies/BuildSystemClient.swift index d44212cb5..4a564ef1b 100644 --- a/Sources/App/Core/Dependencies/BuildSystemClient.swift +++ b/Sources/App/Core/Dependencies/BuildSystemClient.swift @@ -19,27 +19,21 @@ import Vapor @DependencyClient struct BuildSystemClient { - var apiToken: @Sendable () throws -> String #warning("remove client") var getStatusCount: @Sendable (_ client: Client, _ status: Gitlab.Builder.Status) async throws -> Int - var gitlabPipelineToken: @Sendable () -> String? } extension BuildSystemClient: DependencyKey { static var liveValue: Self { .init( - apiToken: { - try Environment.get("GITLAB_API_TOKEN").unwrap(or: Gitlab.Error.missingToken) - }, getStatusCount: { client, status in try await Gitlab.Builder.getStatusCount(client: client, status: status, page: 1, pageSize: 100, maxPageCount: 5) - }, - gitlabPipelineToken: { Environment.get("GITLAB_PIPELINE_TOKEN") } + } ) } } diff --git a/Sources/App/Core/Dependencies/EnvironmentClient.swift b/Sources/App/Core/Dependencies/EnvironmentClient.swift index 836473183..03978ff21 100644 --- a/Sources/App/Core/Dependencies/EnvironmentClient.swift +++ b/Sources/App/Core/Dependencies/EnvironmentClient.swift @@ -41,6 +41,8 @@ struct EnvironmentClient { var collectionSigningPrivateKey: @Sendable () -> Data? var current: @Sendable () -> Environment = { XCTFail("current"); return .development } var dbId: @Sendable () -> String? + var gitlabApiToken: @Sendable () -> String? + var gitlabPipelineToken: @Sendable () -> String? var hideStagingBanner: @Sendable () -> Bool = { XCTFail("hideStagingBanner"); return Constants.defaultHideStagingBanner } var loadSPIManifest: @Sendable (String) -> SPIManifest.Manifest? var maintenanceMessage: @Sendable () -> String? @@ -109,6 +111,8 @@ extension EnvironmentClient: DependencyKey { }, current: { (try? Environment.detect()) ?? .development }, dbId: { Environment.get("DATABASE_ID") }, + gitlabApiToken: { Environment.get("GITLAB_API_TOKEN") }, + gitlabPipelineToken: { Environment.get("GITLAB_PIPELINE_TOKEN") }, hideStagingBanner: { Environment.get("HIDE_STAGING_BANNER").flatMap(\.asBool) ?? Constants.defaultHideStagingBanner diff --git a/Sources/App/Core/Gitlab.swift b/Sources/App/Core/Gitlab.swift index d101da034..6225ea05c 100644 --- a/Sources/App/Core/Gitlab.swift +++ b/Sources/App/Core/Gitlab.swift @@ -78,11 +78,11 @@ extension Gitlab.Builder { swiftVersion: SwiftVersion, versionID: Version.Id) async throws -> Build.TriggerResponse { @Dependency(\.environment) var environment - @Dependency(\.buildSystem) var buildSystem - guard let pipelineToken = buildSystem.gitlabPipelineToken(), + guard let pipelineToken = environment.gitlabPipelineToken(), let builderToken = environment.builderToken() else { throw Gitlab.Error.missingToken } + guard let awsDocsBucket = environment.awsDocsBucket() else { throw Gitlab.Error.missingConfiguration("AWS_DOCS_BUCKET") } @@ -158,9 +158,11 @@ extension Gitlab.Builder { status: Status, page: Int, pageSize: Int = 20) async throws -> [Pipeline] { - @Dependency(\.buildSystem) var buildSystem + @Dependency(\.environment) var environment + guard let apiToken = environment.gitlabApiToken() else { throw Gitlab.Error.missingToken } + let uri: URI = .init(string: "\(projectURL)/pipelines?status=\(status)&page=\(page)&per_page=\(pageSize)") - let response = try await client.get(uri, headers: .bearer(buildSystem.apiToken())) + let response = try await client.get(uri, headers: .bearer(apiToken)) guard response.status == .ok else { throw Gitlab.Error.requestFailed(response.status, uri) } diff --git a/Tests/AppTests/BuildTests.swift b/Tests/AppTests/BuildTests.swift index 9363ae387..2915d39c0 100644 --- a/Tests/AppTests/BuildTests.swift +++ b/Tests/AppTests/BuildTests.swift @@ -130,10 +130,10 @@ class BuildTests: AppTestCase { func test_trigger() async throws { try await withDependencies { - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } } operation: { // setup @@ -198,10 +198,10 @@ class BuildTests: AppTestCase { func test_trigger_isDocBuild() async throws { try await withDependencies { - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } } operation: { // Same test as test_trigger above, except we trigger with isDocBuild: true diff --git a/Tests/AppTests/BuildTriggerTests.swift b/Tests/AppTests/BuildTriggerTests.swift index 304868980..cd003cdfe 100644 --- a/Tests/AppTests/BuildTriggerTests.swift +++ b/Tests/AppTests/BuildTriggerTests.swift @@ -342,10 +342,10 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuildsUnchecked() async throws { try await withDependencies { - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } } operation: { // setup @@ -403,11 +403,11 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuildsUnchecked_supported() async throws { try await withDependencies { - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } $0.environment.buildTriggerAllowList = { [] } + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } } operation: { // Explicitly test the full range of all currently triggered platforms and swift versions @@ -484,10 +484,10 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuildsUnchecked_build_exists() async throws { try await withDependencies { - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } } operation: { // Tests error handling when a build record already exists and `create` raises a @@ -563,13 +563,13 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuilds_checked() async throws { try await withDependencies { - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } $0.environment.buildTriggerAllowList = { [] } $0.environment.buildTriggerDownscaling = { 1 } + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } } operation: { @@ -689,7 +689,6 @@ class BuildTriggerTests: AppTestCase { $0.buildSystem.getStatusCount = { @Sendable c, _ in 299 + triggerCount.withLockedValue { $0 } } - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } @@ -697,6 +696,7 @@ class BuildTriggerTests: AppTestCase { $0.environment.buildTriggerAllowList = { [] } $0.environment.buildTriggerDownscaling = { 1 } $0.environment.buildTriggerLatestSwiftVersionDownscaling = { 1 } + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } } operation: { @@ -743,12 +743,12 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuilds_trimming() async throws { try await withDependencies { $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTriggerAllowList = { [] } $0.environment.buildTriggerDownscaling = { 1 } + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } } operation: { @@ -783,13 +783,13 @@ class BuildTriggerTests: AppTestCase { func test_triggerBuilds_error() async throws { try await withDependencies { $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } $0.environment.buildTriggerAllowList = { [] } $0.environment.buildTriggerDownscaling = { 1 } + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } } operation: { @@ -913,12 +913,12 @@ class BuildTriggerTests: AppTestCase { func test_override_switch() async throws { try await withDependencies { $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } $0.environment.buildTriggerAllowList = { [] } $0.environment.buildTriggerDownscaling = { 1 } + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } } operation: { @@ -991,13 +991,13 @@ class BuildTriggerTests: AppTestCase { func test_downscaling() async throws { try await withDependencies { $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } $0.environment.buildTriggerAllowList = { [] } $0.environment.buildTriggerDownscaling = { 0.05 } // 5% downscaling rate + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } } operation: { // Test build trigger downscaling behaviour @@ -1069,13 +1069,13 @@ class BuildTriggerTests: AppTestCase { func test_downscaling_allow_list_override() async throws { try await withDependencies { $0.buildSystem.getStatusCount = { @Sendable _, _ in 100 } - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.allowBuildTriggers = { true } $0.environment.awsDocsBucket = { "awsDocsBucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } $0.environment.buildTriggerAllowList = { [.id0] } $0.environment.buildTriggerDownscaling = { 0.05 } // 5% downscaling rate + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } } operation: { // Test build trigger downscaling behaviour for allow-listed packages diff --git a/Tests/AppTests/GitlabBuilderTests.swift b/Tests/AppTests/GitlabBuilderTests.swift index 72fb2a02d..b5d8ee661 100644 --- a/Tests/AppTests/GitlabBuilderTests.swift +++ b/Tests/AppTests/GitlabBuilderTests.swift @@ -55,10 +55,10 @@ class GitlabBuilderTests: AppTestCase { func test_triggerBuild() async throws { try await withDependencies { - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "docs-bucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } } operation: { let buildId = UUID() @@ -104,10 +104,10 @@ class GitlabBuilderTests: AppTestCase { func test_issue_588() async throws { try await withDependencies { - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.awsDocsBucket = { "docs-bucket" } $0.environment.builderToken = { "builder token" } $0.environment.buildTimeout = { 10 } + $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } } operation: { var called = false @@ -137,8 +137,8 @@ class GitlabBuilderTests: AppTestCase { func test_getStatusCount() async throws { try await withDependencies { - $0.buildSystem.gitlabPipelineToken = { nil } - $0.buildSystem.apiToken = { "api token" } + $0.environment.gitlabApiToken = { "api token" } +// $0.environment.gitlabPipelineToken = { "pipeline token" } } operation: { var page = 1 let client = MockClient { req, res in @@ -179,16 +179,15 @@ class LiveGitlabBuilderTests: AppTestCase { try await withDependencies { // make sure environment variables are configured for live access - $0.buildSystem.gitlabPipelineToken = { - // This Gitlab token is required in order to trigger the pipeline - ProcessInfo.processInfo.environment["LIVE_GITLAB_PIPELINE_TOKEN"] - } + $0.environment.awsDocsBucket = { "spi-dev-docs" } $0.environment.builderToken = { // Set this to a valid value if you want to report build results back to the server ProcessInfo.processInfo.environment["LIVE_BUILDER_TOKEN"] } - // make sure environment variables are configured for live access - $0.environment.awsDocsBucket = { "spi-dev-docs" } + $0.environment.gitlabPipelineToken = { + // This Gitlab token is required in order to trigger the pipeline + ProcessInfo.processInfo.environment["LIVE_GITLAB_PIPELINE_TOKEN"] + } $0.environment.siteURL = { "https://staging.swiftpackageindex.com" } } operation: { // set build branch to trigger on diff --git a/Tests/AppTests/MetricsTests.swift b/Tests/AppTests/MetricsTests.swift index 1ff0c4faf..ca4f62957 100644 --- a/Tests/AppTests/MetricsTests.swift +++ b/Tests/AppTests/MetricsTests.swift @@ -23,8 +23,8 @@ class MetricsTests: AppTestCase { func test_basic() async throws { try await withDependencies { - $0.buildSystem.gitlabPipelineToken = { "pipeline token" } $0.environment.builderToken = { "builder token" } + $0.environment.gitlabPipelineToken = { "pipeline token" } } operation: { // setup - trigger build to increment counter let versionId = UUID() From 9feeb9c8d7cdda20a67ce4c707b4a51b664367f1 Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Wed, 22 Jan 2025 08:40:56 +0100 Subject: [PATCH 6/8] Cleanup --- Tests/AppTests/GitlabBuilderTests.swift | 1 - 1 file changed, 1 deletion(-) diff --git a/Tests/AppTests/GitlabBuilderTests.swift b/Tests/AppTests/GitlabBuilderTests.swift index b5d8ee661..ce311f662 100644 --- a/Tests/AppTests/GitlabBuilderTests.swift +++ b/Tests/AppTests/GitlabBuilderTests.swift @@ -138,7 +138,6 @@ class GitlabBuilderTests: AppTestCase { func test_getStatusCount() async throws { try await withDependencies { $0.environment.gitlabApiToken = { "api token" } -// $0.environment.gitlabPipelineToken = { "pipeline token" } } operation: { var page = 1 let client = MockClient { req, res in From 8f96e29d2a51210b38d5eb50086eae2efa2b634e Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Wed, 22 Jan 2025 08:54:26 +0100 Subject: [PATCH 7/8] Move Current.triggerBuild to buildSystem.triggerBuild --- Sources/App/Core/AppEnvironment.swift | 20 +--- .../Core/Dependencies/BuildSystemClient.swift | 19 ++++ Sources/App/Models/Build.swift | 19 ++-- Tests/AppTests/BuildTests.swift | 40 ++++---- Tests/AppTests/BuildTriggerTests.swift | 92 +++++++++---------- Tests/AppTests/MetricsTests.swift | 3 + .../AppTests/Mocks/AppEnvironment+mock.swift | 3 +- 7 files changed, 101 insertions(+), 95 deletions(-) diff --git a/Sources/App/Core/AppEnvironment.swift b/Sources/App/Core/AppEnvironment.swift index a8941900f..d3975fd46 100644 --- a/Sources/App/Core/AppEnvironment.swift +++ b/Sources/App/Core/AppEnvironment.swift @@ -29,14 +29,6 @@ struct AppEnvironment: Sendable { var logger: @Sendable () -> Logger var setLogger: @Sendable (Logger) -> Void var shell: Shell - var triggerBuild: @Sendable (_ client: Client, - _ buildId: Build.Id, - _ cloneURL: String, - _ isDocBuild: Bool, - _ platform: Build.Platform, - _ reference: Reference, - _ swiftVersion: SwiftVersion, - _ versionID: Version.Id) async throws -> Build.TriggerResponse } @@ -52,17 +44,7 @@ extension AppEnvironment { }, logger: { logger }, setLogger: { logger in Self.logger = logger }, - shell: .live, - triggerBuild: { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in - try await Gitlab.Builder.triggerBuild(client: client, - buildId: buildId, - cloneURL: cloneURL, - isDocBuild: isDocBuild, - platform: platform, - reference: ref, - swiftVersion: swiftVersion, - versionID: versionID) - } + shell: .live ) } diff --git a/Sources/App/Core/Dependencies/BuildSystemClient.swift b/Sources/App/Core/Dependencies/BuildSystemClient.swift index 4a564ef1b..02abdb17c 100644 --- a/Sources/App/Core/Dependencies/BuildSystemClient.swift +++ b/Sources/App/Core/Dependencies/BuildSystemClient.swift @@ -21,6 +21,15 @@ import Vapor struct BuildSystemClient { #warning("remove client") var getStatusCount: @Sendable (_ client: Client, _ status: Gitlab.Builder.Status) async throws -> Int +#warning("remove client") + var triggerBuild: @Sendable (_ client: Client, + _ buildId: Build.Id, + _ cloneURL: String, + _ isDocBuild: Bool, + _ platform: Build.Platform, + _ reference: Reference, + _ swiftVersion: SwiftVersion, + _ versionID: Version.Id) async throws -> Build.TriggerResponse } @@ -33,6 +42,16 @@ extension BuildSystemClient: DependencyKey { page: 1, pageSize: 100, maxPageCount: 5) + }, + triggerBuild: { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in + try await Gitlab.Builder.triggerBuild(client: client, + buildId: buildId, + cloneURL: cloneURL, + isDocBuild: isDocBuild, + platform: platform, + reference: ref, + swiftVersion: swiftVersion, + versionID: versionID) } ) } diff --git a/Sources/App/Models/Build.swift b/Sources/App/Models/Build.swift index 1b758c597..0cee9f3c4 100644 --- a/Sources/App/Models/Build.swift +++ b/Sources/App/Models/Build.swift @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +import Dependencies import Fluent import PostgresKit import Vapor @@ -186,14 +187,16 @@ extension Build { .with(\.$package) .first() .unwrap(or: Abort(.notFound)) - return try await Current.triggerBuild(client, - buildId, - version.package.url, - isDocBuild, - platform, - version.reference, - swiftVersion, - versionId) + + @Dependency(\.buildSystem) var buildSystem + return try await buildSystem.triggerBuild(client, + buildId, + version.package.url, + isDocBuild, + platform, + version.reference, + swiftVersion, + versionId) } } diff --git a/Tests/AppTests/BuildTests.swift b/Tests/AppTests/BuildTests.swift index 2915d39c0..5d4b1b55b 100644 --- a/Tests/AppTests/BuildTests.swift +++ b/Tests/AppTests/BuildTests.swift @@ -135,17 +135,9 @@ class BuildTests: AppTestCase { $0.environment.buildTimeout = { 10 } $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } - } operation: { - // setup - let p = try await savePackage(on: app.db, "1") - let v = try Version(package: p, reference: .branch("main")) - try await v.save(on: app.db) - let buildId = UUID() - let versionID = try XCTUnwrap(v.id) - // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request - Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in + $0.buildSystem.triggerBuild = { @Sendable client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in try await Gitlab.Builder.triggerBuild(client: client, buildId: buildId, cloneURL: cloneURL, @@ -155,6 +147,14 @@ class BuildTests: AppTestCase { swiftVersion: swiftVersion, versionID: versionID) } + } operation: { + // setup + let p = try await savePackage(on: app.db, "1") + let v = try Version(package: p, reference: .branch("main")) + try await v.save(on: app.db) + let buildId = UUID() + let versionID = try XCTUnwrap(v.id) + var called = false let client = MockClient { req, res in called = true @@ -203,19 +203,9 @@ class BuildTests: AppTestCase { $0.environment.buildTimeout = { 10 } $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } - } operation: { - // Same test as test_trigger above, except we trigger with isDocBuild: true - // and expect a 15m TIMEOUT instead of 10m - // setup - let p = try await savePackage(on: app.db, "1") - let v = try Version(package: p, reference: .branch("main")) - try await v.save(on: app.db) - let buildId = UUID() - let versionID = try XCTUnwrap(v.id) - // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request - Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in + $0.buildSystem.triggerBuild = { @Sendable client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in try await Gitlab.Builder.triggerBuild(client: client, buildId: buildId, cloneURL: cloneURL, @@ -225,6 +215,16 @@ class BuildTests: AppTestCase { swiftVersion: swiftVersion, versionID: versionID) } + } operation: { + // Same test as test_trigger above, except we trigger with isDocBuild: true + // and expect a 15m TIMEOUT instead of 10m + // setup + let p = try await savePackage(on: app.db, "1") + let v = try Version(package: p, reference: .branch("main")) + try await v.save(on: app.db) + let buildId = UUID() + let versionID = try XCTUnwrap(v.id) + var called = false let client = MockClient { req, res in called = true diff --git a/Tests/AppTests/BuildTriggerTests.swift b/Tests/AppTests/BuildTriggerTests.swift index cd003cdfe..ee0bc37fe 100644 --- a/Tests/AppTests/BuildTriggerTests.swift +++ b/Tests/AppTests/BuildTriggerTests.swift @@ -347,11 +347,9 @@ class BuildTriggerTests: AppTestCase { $0.environment.buildTimeout = { 10 } $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } - } operation: { - // setup // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request - Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in + $0.buildSystem.triggerBuild = { @Sendable client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in try await Gitlab.Builder.triggerBuild(client: client, buildId: buildId, cloneURL: cloneURL, @@ -361,6 +359,8 @@ class BuildTriggerTests: AppTestCase { swiftVersion: swiftVersion, versionID: versionID) } + } operation: { + // setup let queries = QueueIsolated<[Gitlab.Builder.PostDTO]>([]) let client = MockClient { req, res in guard let query = try? req.query.decode(Gitlab.Builder.PostDTO.self) else { return } @@ -409,12 +409,9 @@ class BuildTriggerTests: AppTestCase { $0.environment.buildTriggerAllowList = { [] } $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } - } operation: { - // Explicitly test the full range of all currently triggered platforms and swift versions - // setup // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request - Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in + $0.buildSystem.triggerBuild = { @Sendable client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in try await Gitlab.Builder.triggerBuild(client: client, buildId: buildId, cloneURL: cloneURL, @@ -424,6 +421,9 @@ class BuildTriggerTests: AppTestCase { swiftVersion: swiftVersion, versionID: versionID) } + } operation: { + // Explicitly test the full range of all currently triggered platforms and swift versions + // setup let queries = QueueIsolated<[Gitlab.Builder.PostDTO]>([]) let client = MockClient { req, res in guard let query = try? req.query.decode(Gitlab.Builder.PostDTO.self) else { return } @@ -489,6 +489,18 @@ class BuildTriggerTests: AppTestCase { $0.environment.buildTimeout = { 10 } $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } + // Use live dependency but replace actual client with a mock so we can + // assert on the details being sent without actually making a request + $0.buildSystem.triggerBuild = { @Sendable client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in + try await Gitlab.Builder.triggerBuild(client: client, + buildId: buildId, + cloneURL: cloneURL, + isDocBuild: isDocBuild, + platform: platform, + reference: ref, + swiftVersion: swiftVersion, + versionID: versionID) + } } operation: { // Tests error handling when a build record already exists and `create` raises a // uq:builds.version_id+builds.platform+builds.swift_version+v2 @@ -501,18 +513,6 @@ class BuildTriggerTests: AppTestCase { // being completely ignored because the command errors out. // See https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/issues/2237 // setup - // Use live dependency but replace actual client with a mock so we can - // assert on the details being sent without actually making a request - Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in - try await Gitlab.Builder.triggerBuild(client: client, - buildId: buildId, - cloneURL: cloneURL, - isDocBuild: isDocBuild, - platform: platform, - reference: ref, - swiftVersion: swiftVersion, - versionID: versionID) - } let queries = QueueIsolated<[Gitlab.Builder.PostDTO]>([]) let client = MockClient { req, res in guard let query = try? req.query.decode(Gitlab.Builder.PostDTO.self) else { return } @@ -572,13 +572,9 @@ class BuildTriggerTests: AppTestCase { $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } - } operation: { - // Ensure we respect the pipeline limit when triggering builds - // setup - Current.gitlabPipelineLimit = { 300 } // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request - Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in + $0.buildSystem.triggerBuild = { @Sendable client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in try await Gitlab.Builder.triggerBuild(client: client, buildId: buildId, cloneURL: cloneURL, @@ -588,6 +584,10 @@ class BuildTriggerTests: AppTestCase { swiftVersion: swiftVersion, versionID: versionID) } + } operation: { + // Ensure we respect the pipeline limit when triggering builds + // setup + Current.gitlabPipelineLimit = { 300 } var triggerCount = 0 let client = MockClient { _, res in triggerCount += 1 @@ -699,13 +699,9 @@ class BuildTriggerTests: AppTestCase { $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } - } operation: { - // Ensure we respect the pipeline limit when triggering builds for multiple package ids - // setup - Current.gitlabPipelineLimit = { 300 } // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request - Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in + $0.buildSystem.triggerBuild = { @Sendable client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in try await Gitlab.Builder.triggerBuild(client: client, buildId: buildId, cloneURL: cloneURL, @@ -715,6 +711,10 @@ class BuildTriggerTests: AppTestCase { swiftVersion: swiftVersion, versionID: versionID) } + } operation: { + // Ensure we respect the pipeline limit when triggering builds for multiple package ids + // setup + Current.gitlabPipelineLimit = { 300 } let client = MockClient { _, res in triggerCount.withLockedValue { $0 += 1 } try? res.content.encode( @@ -792,13 +792,9 @@ class BuildTriggerTests: AppTestCase { $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } - } operation: { - // Ensure we trim builds as part of triggering - // setup - Current.gitlabPipelineLimit = { 300 } // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request - Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in + $0.buildSystem.triggerBuild = { @Sendable client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in try await Gitlab.Builder.triggerBuild(client: client, buildId: buildId, cloneURL: cloneURL, @@ -808,6 +804,10 @@ class BuildTriggerTests: AppTestCase { swiftVersion: swiftVersion, versionID: versionID) } + } operation: { + // Ensure we trim builds as part of triggering + // setup + Current.gitlabPipelineLimit = { 300 } var triggerCount = 0 let client = MockClient { _, res in // let the 5th trigger succeed to ensure we don't early out on errors @@ -921,12 +921,9 @@ class BuildTriggerTests: AppTestCase { $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } - } operation: { - // Ensure don't trigger if the override is off - // setup // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request - Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in + $0.buildSystem.triggerBuild = { @Sendable client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in try await Gitlab.Builder.triggerBuild(client: client, buildId: buildId, cloneURL: cloneURL, @@ -936,6 +933,9 @@ class BuildTriggerTests: AppTestCase { swiftVersion: swiftVersion, versionID: versionID) } + } operation: { + // Ensure don't trigger if the override is off + // setup var triggerCount = 0 let client = MockClient { _, res in triggerCount += 1 @@ -999,12 +999,9 @@ class BuildTriggerTests: AppTestCase { $0.environment.buildTriggerDownscaling = { 0.05 } // 5% downscaling rate $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } - } operation: { - // Test build trigger downscaling behaviour - // setup // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request - Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in + $0.buildSystem.triggerBuild = { @Sendable client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in try await Gitlab.Builder.triggerBuild(client: client, buildId: buildId, cloneURL: cloneURL, @@ -1014,6 +1011,9 @@ class BuildTriggerTests: AppTestCase { swiftVersion: swiftVersion, versionID: versionID) } + } operation: { + // Test build trigger downscaling behaviour + // setup var triggerCount = 0 let client = MockClient { _, res in triggerCount += 1 @@ -1077,12 +1077,9 @@ class BuildTriggerTests: AppTestCase { $0.environment.buildTriggerDownscaling = { 0.05 } // 5% downscaling rate $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } - } operation: { - // Test build trigger downscaling behaviour for allow-listed packages - // setup // Use live dependency but replace actual client with a mock so we can // assert on the details being sent without actually making a request - Current.triggerBuild = { client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in + $0.buildSystem.triggerBuild = { @Sendable client, buildId, cloneURL, isDocBuild, platform, ref, swiftVersion, versionID in try await Gitlab.Builder.triggerBuild(client: client, buildId: buildId, cloneURL: cloneURL, @@ -1092,6 +1089,9 @@ class BuildTriggerTests: AppTestCase { swiftVersion: swiftVersion, versionID: versionID) } + } operation: { + // Test build trigger downscaling behaviour for allow-listed packages + // setup var triggerCount = 0 let client = MockClient { _, res in triggerCount += 1 diff --git a/Tests/AppTests/MetricsTests.swift b/Tests/AppTests/MetricsTests.swift index ca4f62957..3280be75d 100644 --- a/Tests/AppTests/MetricsTests.swift +++ b/Tests/AppTests/MetricsTests.swift @@ -23,6 +23,9 @@ class MetricsTests: AppTestCase { func test_basic() async throws { try await withDependencies { + $0.buildSystem.triggerBuild = { @Sendable _, _, _, _, _, _, _, _ in + .init(status: .ok, webUrl: "") + } $0.environment.builderToken = { "builder token" } $0.environment.gitlabPipelineToken = { "pipeline token" } } operation: { diff --git a/Tests/AppTests/Mocks/AppEnvironment+mock.swift b/Tests/AppTests/Mocks/AppEnvironment+mock.swift index 25470ed42..187c83397 100644 --- a/Tests/AppTests/Mocks/AppEnvironment+mock.swift +++ b/Tests/AppTests/Mocks/AppEnvironment+mock.swift @@ -27,8 +27,7 @@ extension AppEnvironment { gitlabPipelineLimit: { Constants.defaultGitlabPipelineLimit }, logger: { logger }, setLogger: { logger in Self.logger = logger }, - shell: .mock, - triggerBuild: { _, _, _, _, _, _, _, _ in .init(status: .ok, webUrl: "http://web_url") } + shell: .mock ) } } From 3ebdd66e2b079dd3386a65442c69e64bfea7a253 Mon Sep 17 00:00:00 2001 From: "Sven A. Schmidt" Date: Wed, 22 Jan 2025 09:14:36 +0100 Subject: [PATCH 8/8] Move Current.gitlabPipelineLimit to BuildSystem.gitlabPipelineLimit --- Sources/App/Commands/TriggerBuilds.swift | 5 +++-- Sources/App/Core/AppEnvironment.swift | 5 ----- Sources/App/Core/Dependencies/EnvironmentClient.swift | 5 +++++ Tests/AppTests/BuildTriggerTests.swift | 11 +++++++---- Tests/AppTests/Mocks/AppEnvironment+mock.swift | 1 - 5 files changed, 15 insertions(+), 12 deletions(-) diff --git a/Sources/App/Commands/TriggerBuilds.swift b/Sources/App/Commands/TriggerBuilds.swift index 6cd45c322..4f32270cc 100644 --- a/Sources/App/Commands/TriggerBuilds.swift +++ b/Sources/App/Commands/TriggerBuilds.swift @@ -207,6 +207,7 @@ func triggerBuilds(on database: Database, AppMetrics.buildRunningJobsCount?.set(runningJobs) let newJobs = ActorIsolated(0) + let gitlabPipelineLimit = environment.gitlabPipelineLimit await withThrowingTaskGroup(of: Void.self) { group in for pkgId in packages { @@ -219,7 +220,7 @@ func triggerBuilds(on database: Database, group.addTask { // check if we have capacity to schedule more builds before querying for builds var newJobCount = await newJobs.value - guard pendingJobs + newJobCount < Current.gitlabPipelineLimit() else { + guard pendingJobs + newJobCount < gitlabPipelineLimit() else { Current.logger().info("too many pending pipelines (\(pendingJobs + newJobCount))") return } @@ -228,7 +229,7 @@ func triggerBuilds(on database: Database, let triggers = try await findMissingBuilds(database, packageId: pkgId) newJobCount = await newJobs.value - guard pendingJobs + newJobCount < Current.gitlabPipelineLimit() else { + guard pendingJobs + newJobCount < gitlabPipelineLimit() else { Current.logger().info("too many pending pipelines (\(pendingJobs + newJobCount))") return } diff --git a/Sources/App/Core/AppEnvironment.swift b/Sources/App/Core/AppEnvironment.swift index d3975fd46..bfa4450f5 100644 --- a/Sources/App/Core/AppEnvironment.swift +++ b/Sources/App/Core/AppEnvironment.swift @@ -25,7 +25,6 @@ import FoundationNetworking struct AppEnvironment: Sendable { var fileManager: FileManager var git: Git - var gitlabPipelineLimit: @Sendable () -> Int var logger: @Sendable () -> Logger var setLogger: @Sendable (Logger) -> Void var shell: Shell @@ -38,10 +37,6 @@ extension AppEnvironment { static let live = AppEnvironment( fileManager: .live, git: .live, - gitlabPipelineLimit: { - Environment.get("GITLAB_PIPELINE_LIMIT").flatMap(Int.init) - ?? Constants.defaultGitlabPipelineLimit - }, logger: { logger }, setLogger: { logger in Self.logger = logger }, shell: .live diff --git a/Sources/App/Core/Dependencies/EnvironmentClient.swift b/Sources/App/Core/Dependencies/EnvironmentClient.swift index 03978ff21..46d00bcfe 100644 --- a/Sources/App/Core/Dependencies/EnvironmentClient.swift +++ b/Sources/App/Core/Dependencies/EnvironmentClient.swift @@ -42,6 +42,7 @@ struct EnvironmentClient { var current: @Sendable () -> Environment = { XCTFail("current"); return .development } var dbId: @Sendable () -> String? var gitlabApiToken: @Sendable () -> String? + var gitlabPipelineLimit: @Sendable () -> Int = { XCTFail("gitlabPipelineLimit"); return 100 } var gitlabPipelineToken: @Sendable () -> String? var hideStagingBanner: @Sendable () -> Bool = { XCTFail("hideStagingBanner"); return Constants.defaultHideStagingBanner } var loadSPIManifest: @Sendable (String) -> SPIManifest.Manifest? @@ -112,6 +113,10 @@ extension EnvironmentClient: DependencyKey { current: { (try? Environment.detect()) ?? .development }, dbId: { Environment.get("DATABASE_ID") }, gitlabApiToken: { Environment.get("GITLAB_API_TOKEN") }, + gitlabPipelineLimit: { + Environment.get("GITLAB_PIPELINE_LIMIT").flatMap(Int.init) + ?? Constants.defaultGitlabPipelineLimit + }, gitlabPipelineToken: { Environment.get("GITLAB_PIPELINE_TOKEN") }, hideStagingBanner: { Environment.get("HIDE_STAGING_BANNER").flatMap(\.asBool) diff --git a/Tests/AppTests/BuildTriggerTests.swift b/Tests/AppTests/BuildTriggerTests.swift index ee0bc37fe..8e284474c 100644 --- a/Tests/AppTests/BuildTriggerTests.swift +++ b/Tests/AppTests/BuildTriggerTests.swift @@ -569,6 +569,7 @@ class BuildTriggerTests: AppTestCase { $0.environment.buildTimeout = { 10 } $0.environment.buildTriggerAllowList = { [] } $0.environment.buildTriggerDownscaling = { 1 } + $0.environment.gitlabPipelineLimit = { 300 } $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } @@ -587,7 +588,6 @@ class BuildTriggerTests: AppTestCase { } operation: { // Ensure we respect the pipeline limit when triggering builds // setup - Current.gitlabPipelineLimit = { 300 } var triggerCount = 0 let client = MockClient { _, res in triggerCount += 1 @@ -696,6 +696,7 @@ class BuildTriggerTests: AppTestCase { $0.environment.buildTriggerAllowList = { [] } $0.environment.buildTriggerDownscaling = { 1 } $0.environment.buildTriggerLatestSwiftVersionDownscaling = { 1 } + $0.environment.gitlabPipelineLimit = { 300 } $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } @@ -714,7 +715,6 @@ class BuildTriggerTests: AppTestCase { } operation: { // Ensure we respect the pipeline limit when triggering builds for multiple package ids // setup - Current.gitlabPipelineLimit = { 300 } let client = MockClient { _, res in triggerCount.withLockedValue { $0 += 1 } try? res.content.encode( @@ -748,13 +748,13 @@ class BuildTriggerTests: AppTestCase { $0.environment.builderToken = { "builder token" } $0.environment.buildTriggerAllowList = { [] } $0.environment.buildTriggerDownscaling = { 1 } + $0.environment.gitlabPipelineLimit = { 300 } $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } } operation: { // Ensure we trim builds as part of triggering // setup - Current.gitlabPipelineLimit = { 300 } let client = MockClient { _, _ in } @@ -789,6 +789,7 @@ class BuildTriggerTests: AppTestCase { $0.environment.buildTimeout = { 10 } $0.environment.buildTriggerAllowList = { [] } $0.environment.buildTriggerDownscaling = { 1 } + $0.environment.gitlabPipelineLimit = { 300 } $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } @@ -807,7 +808,6 @@ class BuildTriggerTests: AppTestCase { } operation: { // Ensure we trim builds as part of triggering // setup - Current.gitlabPipelineLimit = { 300 } var triggerCount = 0 let client = MockClient { _, res in // let the 5th trigger succeed to ensure we don't early out on errors @@ -918,6 +918,7 @@ class BuildTriggerTests: AppTestCase { $0.environment.buildTimeout = { 10 } $0.environment.buildTriggerAllowList = { [] } $0.environment.buildTriggerDownscaling = { 1 } + $0.environment.gitlabPipelineLimit = { Constants.defaultGitlabPipelineLimit } $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.random = { @Sendable _ in 0 } $0.environment.siteURL = { "http://example.com" } @@ -997,6 +998,7 @@ class BuildTriggerTests: AppTestCase { $0.environment.buildTimeout = { 10 } $0.environment.buildTriggerAllowList = { [] } $0.environment.buildTriggerDownscaling = { 0.05 } // 5% downscaling rate + $0.environment.gitlabPipelineLimit = { Constants.defaultGitlabPipelineLimit } $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } // Use live dependency but replace actual client with a mock so we can @@ -1075,6 +1077,7 @@ class BuildTriggerTests: AppTestCase { $0.environment.buildTimeout = { 10 } $0.environment.buildTriggerAllowList = { [.id0] } $0.environment.buildTriggerDownscaling = { 0.05 } // 5% downscaling rate + $0.environment.gitlabPipelineLimit = { Constants.defaultGitlabPipelineLimit } $0.environment.gitlabPipelineToken = { "pipeline token" } $0.environment.siteURL = { "http://example.com" } // Use live dependency but replace actual client with a mock so we can diff --git a/Tests/AppTests/Mocks/AppEnvironment+mock.swift b/Tests/AppTests/Mocks/AppEnvironment+mock.swift index 187c83397..dd3aabb85 100644 --- a/Tests/AppTests/Mocks/AppEnvironment+mock.swift +++ b/Tests/AppTests/Mocks/AppEnvironment+mock.swift @@ -24,7 +24,6 @@ extension AppEnvironment { .init( fileManager: .mock, git: .mock, - gitlabPipelineLimit: { Constants.defaultGitlabPipelineLimit }, logger: { logger }, setLogger: { logger in Self.logger = logger }, shell: .mock