From 010e60581be16c1b6a763b07ce6b0d521d12cb9f Mon Sep 17 00:00:00 2001 From: Roberto Oliveira Date: Tue, 3 Oct 2023 11:31:22 -0300 Subject: [PATCH 1/7] [ISSUE 461] Fix issue when getting the fork name for the cases where the fork has a different name --- package-lock.json | 4 ++-- package.json | 2 +- src/service/git/git-api-service.ts | 17 +++++++---------- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/package-lock.json b/package-lock.json index bd6935d5..2a9c8bb2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@kie/build-chain-action", - "version": "3.5.4", + "version": "3.5.5", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@kie/build-chain-action", - "version": "3.5.4", + "version": "3.5.5", "license": "ISC", "dependencies": { "@actions/artifact": "^1.1.0", diff --git a/package.json b/package.json index 449c81f7..c26e1bf3 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@kie/build-chain-action", - "version": "3.5.4", + "version": "3.5.5", "description": "Library to execute commands based on github projects dependencies.", "main": "dist/index.js", "author": "", diff --git a/src/service/git/git-api-service.ts b/src/service/git/git-api-service.ts index 9f32a0a8..bde39c49 100644 --- a/src/service/git/git-api-service.ts +++ b/src/service/git/git-api-service.ts @@ -125,15 +125,18 @@ export class GitAPIService { ): Promise { try { // check whether there is a fork with the same name as repo name - const repoName = await this.checkIfRepositoryExists(targetOwner, repo); + this.logger.info(`Checking if ${targetOwner}/${repo} is forked to ${sourceOwner}/${repo}`); + const repoName = await this.checkIfRepositoryExists(sourceOwner, repo); if (repoName) { + this.logger.info(`Found fork in ${sourceOwner}/${repo}`); return repoName; } else if (targetOwner !== sourceOwner) { /** * find repo from fork list. we reach this case only if we are in the edge case where the forked repo's name is different * from the original one */ + this.logger.info(`Fork ${sourceOwner}/${repo} does not exist. Trying to find a fork with a different name in ${sourceOwner}`); const forkName = ( await this.client .rest(targetOwner, repo) @@ -144,17 +147,13 @@ export class GitAPIService { }) ).data; if (forkName) { + this.logger.info(`Found ${sourceOwner}/${forkName} repository as a fork of ${targetOwner}/${repo}`); return forkName; } } throw new NotFoundError(); } catch (err) { - this.logger.error( - this.getErrorMessage( - err, - `Error getting fork name for ${targetOwner}/${repo} where owner is ${sourceOwner}` - ) - ); + this.logger.info(`Could not find a fork name for ${targetOwner}/${repo} where owner is ${sourceOwner}`); throw err; } } @@ -202,9 +201,7 @@ export class GitAPIService { }); return repo; } catch (err) { - this.logger.error( - this.getErrorMessage(err, `Failed to get ${owner}/${repo}.`) - ); + this.logger.debug(`Failed to get ${owner}/${repo}`); return undefined; } } From d060b00c142e5119721eb1157d8efc4146b76cc0 Mon Sep 17 00:00:00 2001 From: Roberto Oliveira Date: Tue, 3 Oct 2023 13:47:33 -0300 Subject: [PATCH 2/7] fix e2e-regression tests --- test/e2e-regression/cli/tests.json | 6 +++--- test/e2e-regression/github-action/tests.json | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/test/e2e-regression/cli/tests.json b/test/e2e-regression/cli/tests.json index f42aac52..256ff8aa 100644 --- a/test/e2e-regression/cli/tests.json +++ b/test/e2e-regression/cli/tests.json @@ -1,17 +1,17 @@ [ { "name": "issue-372", - "cmd": "build-chain build cross_pr -f 'https://raw.githubusercontent.com/kiegroup/droolsjbpm-build-bootstrap/${BRANCH:main}/.ci/compilation-config.yaml' -o bc -u https://github.com/kiegroup/drools/pull/4978 --skipExecution", + "cmd": "build-chain build cross_pr -f 'https://raw.githubusercontent.com/kiegroup/droolsjbpm-build-bootstrap/${BRANCH:main}/.ci/compilation-config.yaml' -o bc -u https://github.com/kiegroup/appformer/pull/1394 --skipExecution", "description": "To test that definition file url placeholders fallback to using default value. Checking whether build succeeded is enough" }, { "name": "issue-401", - "cmd": "build-chain build cross_pr -f 'https://raw.githubusercontent.com/kiegroup/kogito-pipelines/%{process.env.GITHUB_BASE_REF.replace(/(\\d*)\\.(.*)\\.(.*)/g, (m, n1, n2, n3) => `${+n1-7}.${n2}.${n3}`)}/.ci/pull-request-config.yaml' -o bc -u https://github.com/kiegroup/optaplanner/pull/2634 -p kiegroup/optaplanner --skipExecution", + "cmd": "build-chain build cross_pr -f 'https://raw.githubusercontent.com/apache/incubator-kie-kogito-pipelines/%{process.env.GITHUB_BASE_REF.replace(/(\\d*)\\.(.*)\\.(.*)/g, (m, n1, n2, n3) => `${+n1-7}.${n2}.${n3}`)}/.ci/pull-request-config.yaml' -o bc -u https://github.com/apache/incubator-kie-optaplanner/pull/2634 -p kiegroup/optaplanner --skipExecution", "description": "To test that definition file url placeholders as well as expressions work when defined together. Checking whether build succeeded is enough" }, { "name": "issue-386", - "cmd": "build-chain build cross_pr -f 'https://raw.githubusercontent.com/kiegroup/kogito-pipelines/1.34.x/.ci/pull-request-config.yaml' -o bc -u https://github.com/kiegroup/kogito-examples/pull/1570 -p kiegroup/kogito-examples --skipExecution", + "cmd": "build-chain build cross_pr -f 'https://raw.githubusercontent.com/apache/incubator-kie-kogito-pipelines/1.34.x/.ci/pull-request-config.yaml' -o bc -u https://github.com/apache/incubator-kie-kogito-examples/pull/1570 -p kiegroup/kogito-examples --skipExecution", "description": "To test if GITHUB_BASE_REF is set during CLI execution. Checking if 8.34.x is checked out of drools should verify this", "matchOutput": [ "Project taken from kiegroup/drools:8.34.x", diff --git a/test/e2e-regression/github-action/tests.json b/test/e2e-regression/github-action/tests.json index adf982ce..c53316ea 100644 --- a/test/e2e-regression/github-action/tests.json +++ b/test/e2e-regression/github-action/tests.json @@ -3,18 +3,18 @@ "name": "issue-372", "definition-file": "https://raw.githubusercontent.com/kiegroup/droolsjbpm-build-bootstrap/${BRANCH:main}/.ci/compilation-config.yaml", "flow-type": "cross_pr", - "starting-project": "kiegroup/drools", + "starting-project": "kiegroup/appformer", "skip-execution": "true", - "event": "https://github.com/kiegroup/drools/pull/4978", + "event": "https://github.com/kiegroup/appformer/pull/1394", "description": "To test that definition file url placeholders fallback to using default value. Checking whether build succeeded is enough" }, { "name": "issue-401", - "definition-file": "https://raw.githubusercontent.com/kiegroup/kogito-pipelines/%{process.env.GITHUB_BASE_REF.replace(/(\\d*)\\.(.*)\\.(.*)/g, (m, n1, n2, n3) => `${+n1-7}.${n2}.${n3}`)}/.ci/pull-request-config.yaml", + "definition-file": "https://raw.githubusercontent.com/apache/incubator-kie-kogito-pipelines/%{process.env.GITHUB_BASE_REF.replace(/(\\d*)\\.(.*)\\.(.*)/g, (m, n1, n2, n3) => `${+n1-7}.${n2}.${n3}`)}/.ci/pull-request-config.yaml", "starting-project": "kiegroup/optaplanner", "flow-type": "cross_pr", "skip-execution": "true", - "event": "https://github.com/kiegroup/optaplanner/pull/2634", + "event": "https://github.com/apache/incubator-kie-optaplanner/pull/2634", "description": "To test that definition file url placeholders as well as expressions work when defined together. Checking whether build succeeded is enough" }, { From 2932ab63d3f3d5c34d43e7ba5ad2814bd7568e3b Mon Sep 17 00:00:00 2001 From: Roberto Oliveira Date: Wed, 4 Oct 2023 09:47:58 -0300 Subject: [PATCH 3/7] fix e2e test --- test/e2e/cross-pr/cross-pr.test.ts | 17 +++++++++++++---- .../e2e/full-downstream/full-downstream.test.ts | 17 +++++++++++++---- test/e2e/single-pr/single-pr.test.ts | 6 +++--- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/test/e2e/cross-pr/cross-pr.test.ts b/test/e2e/cross-pr/cross-pr.test.ts index 5e40ef09..00831b62 100644 --- a/test/e2e/cross-pr/cross-pr.test.ts +++ b/test/e2e/cross-pr/cross-pr.test.ts @@ -158,13 +158,22 @@ test("PR from owner1/target:branchA to owner2/target:branchB while using mapping mockApi: [ moctokit.rest.repos .get({ - owner: "owner1", - repo: /project(1|2|4)/, + owner: "owner2", + repo: /project(1|2)/, }) .setResponse({ status: 404, data: {}, - repeat: 3 + repeat: 2 + }), + moctokit.rest.repos + .get({ + owner: "owner2", + repo: "project4" + }) + .setResponse({ + status: 200, + data: {}, }), moctokit.rest.repos .listForks({ @@ -184,7 +193,7 @@ test("PR from owner1/target:branchA to owner2/target:branchB while using mapping }), moctokit.rest.repos .get({ - owner: "owner1", + owner: "owner2", repo: "project3", }) .setResponse({ diff --git a/test/e2e/full-downstream/full-downstream.test.ts b/test/e2e/full-downstream/full-downstream.test.ts index e29ebe77..f480993a 100644 --- a/test/e2e/full-downstream/full-downstream.test.ts +++ b/test/e2e/full-downstream/full-downstream.test.ts @@ -187,13 +187,22 @@ test("PR from owner1/target:branchA to owner2/target:branchB while using mapping mockApi: [ moctokit.rest.repos .get({ - owner: "owner1", - repo: /project(1|2|3|4)/, + owner: "owner2", + repo: "project2", }) .setResponse({ status: 404, data: {}, - repeat: 4 + }), + moctokit.rest.repos + .get({ + owner: "owner2", + repo: /project(1|3|4)/, + }) + .setResponse({ + status: 200, + data: {}, + repeat: 3 }), moctokit.rest.repos .listForks({ @@ -313,7 +322,7 @@ test("PR from owner1/target:branchA to owner2/target:branchB while using mapping ); expect(group4.output).toEqual( expect.stringContaining( - "Merged owner1/project4:branchA into branch 8.x" + "Merged owner2/project4:branchA into branch 8.x" ) ); diff --git a/test/e2e/single-pr/single-pr.test.ts b/test/e2e/single-pr/single-pr.test.ts index 3fb56958..e83b6a74 100644 --- a/test/e2e/single-pr/single-pr.test.ts +++ b/test/e2e/single-pr/single-pr.test.ts @@ -262,7 +262,7 @@ test("PR from owner2/target:branchA to owner1/target:branchB", async () => { mockApi: [ moctokit.rest.repos .get({ - owner: "owner1", + owner: "owner2", repo: "project3", }) .setResponse({ @@ -270,7 +270,7 @@ test("PR from owner2/target:branchA to owner1/target:branchB", async () => { data: { name: "project3", owner: { - login: "owner1", + login: "owner2", }, }, }), @@ -391,7 +391,7 @@ test("PR from owner2/target:branchA to owner1/target-different-name:branchB", as mockApi: [ moctokit.rest.repos .get({ - owner: "owner1", + owner: "owner2", repo: "project1", }) .setResponse({ From 503f39d1d3de81d354275402be8fc0d13881a83b Mon Sep 17 00:00:00 2001 From: Roberto Oliveira Date: Wed, 4 Oct 2023 19:54:54 -0300 Subject: [PATCH 4/7] add unitary test for fork repository with different name --- test/unitary/service/git/git-api.test.ts | 37 ++++++++++++++++-------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/test/unitary/service/git/git-api.test.ts b/test/unitary/service/git/git-api.test.ts index ab48c86b..5a3720ad 100644 --- a/test/unitary/service/git/git-api.test.ts +++ b/test/unitary/service/git/git-api.test.ts @@ -119,22 +119,32 @@ test.each([ [ "success: same source and target owner", true, - ["target1", "target1", "repoA"], + ["target1", "repoA", "target1", "repoA"], ], [ "failure: same source and target owner", false, - ["target2", "target2", "repoA"], + ["target2", "repoA", "target2", "repoA"], ], [ "success: different source and target owner", true, - ["target3", "source", "repoA"], + ["target3", "repoA", "source", "repoA"], ], [ "failure: different source and target owner", false, - ["target4", "source", "repoA"], + ["target4", "repoA", "source", "repoA"], + ], + [ + "success: different source and target owner / different repo name", + true, + ["target5", "repoA", "source", "repoA_fork"], + ], + [ + "failure: different source and target owner / different repo name", + false, + ["target6", "repoA", "source", "no_fork"], ], ])( "getForkName %p", @@ -142,26 +152,29 @@ test.each([ const moctokit = new Moctokit(); if (testForSuccess) { moctokit.rest.repos - .get({ owner: args[1], repo: args[2] }) + .get({ owner: args[0], repo: args[1] }) + .reply({ status: 200, data: {} }); + moctokit.rest.repos + .get({ owner: args[2], repo: args[3] }) .reply({ status: 200, data: {} }); - moctokit.rest.repos.listForks({ owner: args[0], repo: args[2] }).reply({ + moctokit.rest.repos.listForks({ owner: args[0], repo: args[1] }).reply({ status: 200, - data: [{ name: args[2], owner: { login: args[1] } }], + data: [{ name: args[3], owner: { login: args[2] } }], }); - await expect(git.getForkName(args[0], args[1], args[2])).resolves.toBe( - args[2] + await expect(git.getForkName(args[0], args[2], args[1])).resolves.toBe( + args[3] ); } else { moctokit.rest.repos - .get({ owner: args[1], repo: args[2] }) + .get({ owner: args[0], repo: args[1] }) .reply({ status: 404, data: {} }); moctokit.rest.repos - .listForks({ owner: args[0], repo: args[2] }) + .listForks({ owner: args[2], repo: args[3] }) .reply({ status: 200, data: [] }); await expect( - git.getForkName(args[0], args[1], args[2]) + git.getForkName(args[0], args[2], args[1]) ).rejects.toThrowError(); } } From f4bc4834963df5b8e1904961ead73ce58d8f6bdd Mon Sep 17 00:00:00 2001 From: Roberto Oliveira Date: Thu, 5 Oct 2023 07:01:40 -0300 Subject: [PATCH 5/7] Apply suggestions from code review Co-authored-by: Enrique Mingorance Cano --- src/service/git/git-api-service.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/service/git/git-api-service.ts b/src/service/git/git-api-service.ts index bde39c49..ab309923 100644 --- a/src/service/git/git-api-service.ts +++ b/src/service/git/git-api-service.ts @@ -129,7 +129,7 @@ export class GitAPIService { const repoName = await this.checkIfRepositoryExists(sourceOwner, repo); if (repoName) { - this.logger.info(`Found fork in ${sourceOwner}/${repo}`); + this.logger.info(`Fork ${sourceOwner}/${repo} found.`); return repoName; } else if (targetOwner !== sourceOwner) { /** @@ -147,13 +147,13 @@ export class GitAPIService { }) ).data; if (forkName) { - this.logger.info(`Found ${sourceOwner}/${forkName} repository as a fork of ${targetOwner}/${repo}`); + this.logger.info(`Fork ${sourceOwner}/${forkName} found from ${targetOwner}/${repo}`); return forkName; } } throw new NotFoundError(); } catch (err) { - this.logger.info(`Could not find a fork name for ${targetOwner}/${repo} where owner is ${sourceOwner}`); + this.logger.info(`Fork for ${targetOwner}/${repo} not found where owner is ${sourceOwner}`); throw err; } } @@ -201,7 +201,7 @@ export class GitAPIService { }); return repo; } catch (err) { - this.logger.debug(`Failed to get ${owner}/${repo}`); + this.logger.debug(`Failed to get ${owner}/${repo}, it is not necessarily and error`); return undefined; } } From 10f1a13537ebe889bb8bc051b473a89ef51ffc9b Mon Sep 17 00:00:00 2001 From: Roberto Oliveira Date: Thu, 5 Oct 2023 07:13:26 -0300 Subject: [PATCH 6/7] fix typo in e2e test --- test/e2e/cross-pr/cross-pr.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/cross-pr/cross-pr.test.ts b/test/e2e/cross-pr/cross-pr.test.ts index 00831b62..63c34ef4 100644 --- a/test/e2e/cross-pr/cross-pr.test.ts +++ b/test/e2e/cross-pr/cross-pr.test.ts @@ -198,7 +198,7 @@ test("PR from owner1/target:branchA to owner2/target:branchB while using mapping }) .setResponse({ status: 200, - data: { name: "project3", owner: { login: "owner1" } }, + data: { name: "project3", owner: { login: "owner2" } }, }), moctokit.rest.repos .listForks({ From 711a19a4f5b8ee8b55e5eed709a5e981224b11ed Mon Sep 17 00:00:00 2001 From: Roberto Oliveira Date: Thu, 5 Oct 2023 07:50:14 -0300 Subject: [PATCH 7/7] fix typo in debug message --- src/service/git/git-api-service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/service/git/git-api-service.ts b/src/service/git/git-api-service.ts index ab309923..e731b6d5 100644 --- a/src/service/git/git-api-service.ts +++ b/src/service/git/git-api-service.ts @@ -201,7 +201,7 @@ export class GitAPIService { }); return repo; } catch (err) { - this.logger.debug(`Failed to get ${owner}/${repo}, it is not necessarily and error`); + this.logger.debug(`Failed to get ${owner}/${repo}, it is not necessarily an error`); return undefined; } }