[ISSUE 461] Fix issue when getting the fork name for the cases where the fork has a different name #462

rgdoliveira · 2023-10-03T15:14:40Z

Fixes #461

Note: Please, do not check dist/index.js changes. It is automatically generated.

rgdoliveira · 2023-10-03T15:25:18Z

e2e tests are failing, I will investigate why...

rgdoliveira · 2023-10-03T23:35:28Z

src/service/git/git-api-service.ts

@@ -125,15 +125,18 @@ export class GitAPIService {
  ): Promise<string> {
    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);


@jstastny-cz @Ginxo TBH I'm not sure if using checkIfRepositoryExists with sourceOwner instead of targetOwner is the correct way to go, after this change I'm getting many failures in the e2e tests locally and seems that in GH they are running forever and never finish.

To run them locally I used:

$ npm ci $ ACT_LOG=true npm run test:e2e

Actually taking a look in the existing single-pr.test.ts e2e test, there is test PR from owner2/target:branchA to owner1/target-different-name:branchB which should be testing the case where the fork has a different name from the original repository and the test is passing. Am I missing something?

that's it, this functionality is already covered and it shouldn't be re-adapted unless we discover specific corner case for it I'm not able to find (can't get trace logs).
I'm afraid we are missing something on the BC configuration rather than BC bug, but it is just a feeling I would like to confirm/discard. Could you please provide logs URL? Many thanks

Does it really seem correct to you @Ginxo ? We want to check here in the method if there's a fork with the same name as repo name ... but why would we check that the repo in target exists as the first thing and if so then return?

In code we invoke getForkName(currentTarget.group, originalSource.group!, currentTarget.name) in checkout-service.ts:L163, where the currentTarget variable in same file L140 is commented as

// map the starting project target branch to the corresponding branch defined in the mapping for the current node // target branch is guaranteed to exist since base always exist

That really sounds like identity check. And the single-pr.test.ts - I am gonna check, but in this case I have mutliple PRs across 3 repositories, maybe it tests a different thing?

It certainly does not do what the comment just above it says // check whether there is a fork with the same name as repo name

Consider this reproducer :

test("getForkNameForDifferentName", async () => { const moctokit = new Moctokit(); moctokit.rest.repos .get({ owner: "sourceOwner", repo: "repo" }) .reply({ status: 200, data: {} }); moctokit.rest.repos .get({ owner: "targetOwner", repo: "repo" }) .reply({ status: 200, data: { name: "repo", owner: { login: "targetOwner" } } }); moctokit.rest.repos.listForks({ owner: "targetOwner", repo: "repo" }).reply({ status: 200, data: [{ name: "repo-fork", owner: { login: "sourceOwner" } }], }); await expect(git.getForkName("targetOwner", "sourceOwner", "repo")).resolves.toBe( "repo-fork" ); })

getForkName returns "repo".

jstastny-cz · 2023-10-04T13:18:23Z

@shubhbapna if you're available to discuss.

jstastny-cz

I would maybe suggest removing the optimization of expecting that repositories with matching names between various groups/authors are possible forks altogether and rather rely on the resolution.

shubhbapna · 2023-10-04T14:09:13Z

@jstastny-cz You are correct, it should be sourceOwner (the e2e test will need to be fixed. it should probably just involve fixing the right API being mocked). As for removing this optimization, I would advice against it. We were getting rate limit errors almost every other day without this optimization (original PR: #411). Unless there has been a change in the naming convention for most of the forked repositories in apache for kiegroup projects and they don't follow the heuristic that we noticed in the originally.

…the fork has a different name

rgdoliveira · 2023-10-04T14:25:52Z

@shubhbapna is it possible that you help us here related to the e2e tests please?

As part of this PR there is a commit fixing the e2e-regression tests that broke because some repos were migrated from kiegroup to Apache. (This one I think is correct)

And now I'm trying to fix the e2e tests, but I'm struggling with them. I fixed the single-pr.test.ts and just pushed a commit now but I'm not sure if this is actually the correct solution. There are still the full-downstream.test.ts and cross-pr.test.ts that are failing, so if could please help/advice. Thanks

rgdoliveira · 2023-10-04T20:26:55Z

So based on @shubhbapna comment, we followed the right path using sourceOwner when getting the fork name.

All the e2e tests passed no and I would like to ask @shubhbapna @jstastny-cz and @Ginxo to review the PR. Thanks!

jstastny-cz

Would you mind adding the unit test I pasted above? Or I can send a PR to your fork.
I'll try to understand the changes in tests tomorrow and add proper review.

rgdoliveira · 2023-10-04T23:10:34Z

@jstastny-cz there was already a test for the getForkName(), so I expanded it to also include what you suggested, please take a look.

Ginxo

LGTM, just few minor suggestions

src/service/git/git-api-service.ts

jstastny-cz

I think one test mocked values are not corresponding.

test/e2e/cross-pr/cross-pr.test.ts

jstastny-cz · 2023-10-05T08:06:03Z

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",


Ok this seems like it was a hack to get around the issue that we're fixing (to get into the other path of the getForkName method). Seems valid not to fail resolution of the repos.
I would add though a successful resolution of owner1/project4 - that's the one actually receiving a PR. Or do I miss sth?

we already have a successful resolution of owner2/project4 in the block after this one.

In this cross-pr.test.ts, Owner1 has project1, project2, project3 and project4 while Owner 2 has only project3 and project4, that's why I mocked project1 and 2 to fail state and project 3 and 4 to ok state.

I was talking about owner1/project4 , it should not affect how the test runs now, but it might "strengthen" the test context ... anyway, just a minor note really.

@jstastny-cz I would go without is for now, as AFAIK we are not doing it for the other tests like full-downstream.test.ts, otherwise I think we would need to apply this everywhere to be consistent (even not affecting the test result)

@jstastny-cz if you are ok with the PR state, please approve it

Co-authored-by: Enrique Mingorance Cano <ginxaco@gmail.com>

src/service/git/git-api-service.ts

jstastny-cz

LGTM. Thank you.

rgdoliveira requested review from jstastny-cz, mareknovotny and Ginxo October 3, 2023 15:15

rgdoliveira force-pushed the ISSUE-461 branch 3 times, most recently from 763d3f4 to d56870c Compare October 3, 2023 23:24

rgdoliveira commented Oct 3, 2023

View reviewed changes

This was referenced Oct 4, 2023

[DO-NOT-MERGE] [DROOLS-7570] Hang in JpaPersistentStatefulSessionTest apache/incubator-kie-drools#5545

Closed

[DROOLS-7570] Hang in JpaPersistentStatefulSessionTest apache/incubator-kie-drools#5544

Closed

jstastny-cz suggested changes Oct 4, 2023

View reviewed changes

rgdoliveira added 2 commits October 4, 2023 11:17

[ISSUE 461] Fix issue when getting the fork name for the cases where …

010e605

…the fork has a different name

fix e2e-regression tests

d060b00

rgdoliveira force-pushed the ISSUE-461 branch from d56870c to 3646062 Compare October 4, 2023 14:20

rgdoliveira requested a review from shubhbapna October 4, 2023 14:38

rgdoliveira force-pushed the ISSUE-461 branch from 3646062 to 8194626 Compare October 4, 2023 15:36

fix e2e test

2932ab6

rgdoliveira force-pushed the ISSUE-461 branch from 8194626 to 2932ab6 Compare October 4, 2023 20:13

rgdoliveira requested a review from jstastny-cz October 4, 2023 20:24

jstastny-cz reviewed Oct 4, 2023

View reviewed changes

add unitary test for fork repository with different name

503f39d

rgdoliveira requested a review from jstastny-cz October 4, 2023 23:12

Ginxo approved these changes Oct 5, 2023

View reviewed changes

src/service/git/git-api-service.ts Outdated Show resolved Hide resolved

src/service/git/git-api-service.ts Outdated Show resolved Hide resolved

src/service/git/git-api-service.ts Outdated Show resolved Hide resolved

src/service/git/git-api-service.ts Outdated Show resolved Hide resolved

jstastny-cz suggested changes Oct 5, 2023

View reviewed changes

Apply suggestions from code review

f4bc483

Co-authored-by: Enrique Mingorance Cano <ginxaco@gmail.com>

fix typo in e2e test

10f1a13

rgdoliveira requested a review from jstastny-cz October 5, 2023 10:24

rgdoliveira commented Oct 5, 2023

View reviewed changes

src/service/git/git-api-service.ts Outdated Show resolved Hide resolved

fix typo in debug message

711a19a

jstastny-cz approved these changes Oct 5, 2023

View reviewed changes

rgdoliveira merged commit da21001 into kiegroup:main Oct 5, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE 461] Fix issue when getting the fork name for the cases where the fork has a different name #462

[ISSUE 461] Fix issue when getting the fork name for the cases where the fork has a different name #462

rgdoliveira commented Oct 3, 2023

rgdoliveira commented Oct 3, 2023

rgdoliveira Oct 3, 2023

Ginxo Oct 4, 2023 •

edited

jstastny-cz Oct 4, 2023

jstastny-cz Oct 4, 2023

jstastny-cz Oct 4, 2023

jstastny-cz commented Oct 4, 2023

jstastny-cz left a comment

shubhbapna commented Oct 4, 2023

rgdoliveira commented Oct 4, 2023 •

edited

rgdoliveira commented Oct 4, 2023

jstastny-cz left a comment

rgdoliveira commented Oct 4, 2023

Ginxo left a comment

jstastny-cz left a comment

jstastny-cz Oct 5, 2023

rgdoliveira Oct 5, 2023 •

edited

jstastny-cz Oct 5, 2023

rgdoliveira Oct 5, 2023 •

edited

rgdoliveira Oct 5, 2023

jstastny-cz left a comment

[ISSUE 461] Fix issue when getting the fork name for the cases where the fork has a different name #462

[ISSUE 461] Fix issue when getting the fork name for the cases where the fork has a different name #462

Conversation

rgdoliveira commented Oct 3, 2023

rgdoliveira commented Oct 3, 2023

rgdoliveira Oct 3, 2023

Choose a reason for hiding this comment

Ginxo Oct 4, 2023 • edited

Choose a reason for hiding this comment

jstastny-cz Oct 4, 2023

Choose a reason for hiding this comment

jstastny-cz Oct 4, 2023

Choose a reason for hiding this comment

jstastny-cz Oct 4, 2023

Choose a reason for hiding this comment

jstastny-cz commented Oct 4, 2023

jstastny-cz left a comment

Choose a reason for hiding this comment

shubhbapna commented Oct 4, 2023

rgdoliveira commented Oct 4, 2023 • edited

rgdoliveira commented Oct 4, 2023

jstastny-cz left a comment

Choose a reason for hiding this comment

rgdoliveira commented Oct 4, 2023

Ginxo left a comment

Choose a reason for hiding this comment

jstastny-cz left a comment

Choose a reason for hiding this comment

jstastny-cz Oct 5, 2023

Choose a reason for hiding this comment

rgdoliveira Oct 5, 2023 • edited

Choose a reason for hiding this comment

jstastny-cz Oct 5, 2023

Choose a reason for hiding this comment

rgdoliveira Oct 5, 2023 • edited

Choose a reason for hiding this comment

rgdoliveira Oct 5, 2023

Choose a reason for hiding this comment

jstastny-cz left a comment

Choose a reason for hiding this comment

Ginxo Oct 4, 2023 •

edited

rgdoliveira commented Oct 4, 2023 •

edited

rgdoliveira Oct 5, 2023 •

edited

rgdoliveira Oct 5, 2023 •

edited