Update p4 to 1.12

[jasonhawkharris]

• reference issue: Upgrade p4-fusion to version 1.12 #44030

Test plan

Followed the test plan for upgrading p4

[mollylogue]

My only concern is if this change will cause any unexpected regressions, meaning we should run it locally to validate that it doesn't break anything. As of right now I don't believe we have any automated testing around p4-fusion. Our current integration test setup just uses the git-p4 path (we should probably have an issue to remedy that). I think in this case it's probably fine to just merge it, since the changes between this version and the previous one are super small. I'll ask @ryanslade for a second opinion, though.

[ryanslade]

Looks good, but I think this is a good opportunity to create an integration test. We can get that merged and passing and then check this branch against it.

Here's an issue

[ryanslade]

Thanks for making this change 👍

I've added an integration test for this so I'm going to merge it in and test this branch so that we have more confidence that everything is ok.

[ryanslade]

Integration tests running here:
https://buildkite.com/sourcegraph/sourcegraph/builds/183261

It passed, so I think we're good ✅

[ryanslade]

Thanks!

[mrnugget]

Integration tests running here: https://buildkite.com/sourcegraph/sourcegraph/builds/183261

It passed, so I think we're good ✅

How confident are we in those tests? Do we have more P4 fusion tests than the one added here? #44346 (comment) Because I don't think in its current form it tests what we want (I don't think it tests the cloning/fetching at all).

---

@jasonhawkharris did you try this out locally to make sure it works?

I want to know with certainty that we know that this update doesn't break cloning/fetching before we merge it.

[ryanslade]

Lets hold off merging this until we have an integration test in place that confirms we can clone a repo using p4-fusion. The test @mrnugget linked above doesn't test things 100% since the repo may already exist on disk from a previous test. I'm working on another PR that ensures the repo is not on disk first and I'm actually seeing a failure there. I'm pretty sure it's just an issue with the test itself, but still best to be safe.

[jasonhawkharris]

Looks like there are indeed some issues @mrnugget:

[jasonhawkharris]

Here is my Perforce config in external-services-config.json.

"PERFORCE": [
    {
      "p4.port": "perforce.sgdev.org:1666",
      "p4.user": "admin",
      "p4.passwd": "<redacted>",
      "depots": ["//test-perms/", "//mike-test/", "//molly-test/"],
      "repositoryPathPattern": "perforce/{depot}",
    }
  ]

let me know if I've misconfigured something.

[ryanslade]

I had this problem too, hopefully this helps:

sourcegraph/handbook#5573

[ryanslade]

Ok, I've updated the tests so that they remove the repo before tying to clone them, here's a CI run using the new tests:

https://buildkite.com/sourcegraph/sourcegraph/builds/183472 ✅

[jasonhawkharris]

@ryanslade said the test passed. Do you feel good about merging, @mrnugget?

[mrnugget]

No, I'm still not confident in that the test actually tests what we want: https://github.com/sourcegraph/sourcegraph/pull/44352/files#r1023870178

[twarit-waikar]

p4-fusion has a few tests running on Github Actions that validate some of its internal functionalities. It is certainly not fully there yet (we don't even test connecting Perforce from within the tool) but I do plan to build more Perforce API tests for p4-fusion as a part of some ongoing work.

However, seeing an integration test from the Sourcegraph side is great! More testing == Less anxiety for me when I make a new p4-fusion release :D

[mrnugget]

In order to see whether the integration test added in #44352 actually fails if something's broken I created a commit in a branch that replaces the username in the p4 fusion clone command with a wrong username: cecfe56 With that change the tests still pass: https://buildkite.com/sourcegraph/sourcegraph/builds/184040#01848477-eae4-41da-814b-1b914619a360

Maybe that's because p4-fusion uses the P4USER env var with a higher precedence than the --user flag?

So then I went nuclear and replaced the p4 fusion clone command with hello world: 3033610

And guess what?

The integration test still passes.

What am I missing?

[ryanslade]

So then I went nuclear and replaced the p4 fusion clone command with hello world: 3033610

I didn't realise you were doing that so was actually trying the same thing at the same time :) I also saw the test pass.

All I can think of is that maybe it's running the Fetch command instead of clone so I changed that too and re-running integration tests.

[twarit-waikar]

Just to provide data to compare - this is the output you should see with exit code 1 from p4-fusion if you pass in --user WRONG_USER.

Details

[ PRINT @ Main:51 ] p4-fusion v1.12.0
[ PRINT @ Main:73 ] Running p4-fusion from: ./build/p4-fusion/p4-fusion
[ SUCCESS @ InitializeLibraries:160 ] Initialized P4Libraries successfully
[ ERROR @ HandleError:13 ] Received error: error Access for user 'WRONG_USER' has not been enabled by 'p4 protect'.

[ ERROR @ RunEx:118 ] Connection dropped or command errored, retrying in 5 seconds.
[ SUCCESS @ RunEx:123 ] Reinitialized P4API
[ WARNING @ RunEx:130 ] Retrying: p4 changes -m 1 //...
[ ERROR @ HandleError:13 ] Received error: error Access for user 'WRONG_USER' has not been enabled by 'p4 protect'.

[ ERROR @ RunEx:118 ] Connection dropped or command errored, retrying in 5 seconds.
[ SUCCESS @ RunEx:123 ] Reinitialized P4API
[ WARNING @ RunEx:130 ] Retrying: p4 changes -m 1 //...
[ ERROR @ HandleError:13 ] Received error: error Access for user 'WRONG_USER' has not been enabled by 'p4 protect'.

[ ERROR @ RunEx:118 ] Connection dropped or command errored, retrying in 5 seconds.
[ SUCCESS @ RunEx:123 ] Reinitialized P4API
[ WARNING @ RunEx:130 ] Retrying: p4 changes -m 1 //...
[ ERROR @ HandleError:13 ] Received error: error Access for user 'WRONG_USER' has not been enabled by 'p4 protect'.

[ ERROR @ RunEx:118 ] Connection dropped or command errored, retrying in 5 seconds.
[ SUCCESS @ RunEx:123 ] Reinitialized P4API
[ WARNING @ RunEx:130 ] Retrying: p4 changes -m 1 //...
[ ERROR @ HandleError:13 ] Received error: error Access for user 'WRONG_USER' has not been enabled by 'p4 protect'.

[ ERROR @ RunEx:118 ] Connection dropped or command errored, retrying in 5 seconds.
[ SUCCESS @ RunEx:123 ] Reinitialized P4API
[ WARNING @ RunEx:130 ] Retrying: p4 changes -m 1 //...
[ ERROR @ HandleError:13 ] Received error: error Access for user 'WRONG_USER' has not been enabled by 'p4 protect'.

[ ERROR @ Main:94 ] Error occurred while connecting to ssl:REDACTED:1666

Also, p4-fusion expects these required arguments:

❯ ./build/p4-fusion/p4-fusion| grep Required
--client [Required]
--lookAhead [Required]
--path [Required]
--port [Required]
--src [Required]
--user [Required]

If you miss any of these arguments, p4-fusion will fall back to printing the help text, and thus it exits with a 0 exit code. That might be the reason why the test is not failing.

[ryanslade]

Ok, merged some better tests. Will marge main back in here and re-run against the new integration tests.

[ryanslade]

Latest integration tests:
https://buildkite.com/sourcegraph/sourcegraph/builds/184147

[ryanslade]

@jasonhawkharris Can you add a changelog entry for this?

[jasonhawkharris]

@ryanslade #44567

[ryanslade]

@jasonhawkharris Thanks, but I think it can be in this same PR, no need to have a second one.

[mrnugget]

If we now have useful tests on main and they're running on this branch and are not breaking, then let's go ahead an merge this. Thanks!