Bug 234882 – [EWS] Always invoke CleanGitRepo before CheckOutSource

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

As we move to GitHub, all EWS factories should invoke CleanGitRepo and CleanGitRepo should support multiple branches.

[Radar WebKit Bug Importer]

<rdar://problem/87145396>

[Jonathan Bedard]

Created attachment 448387 [details]
Patch

[Jonathan Bedard]

Created attachment 448390 [details]
Patch

[Jonathan Bedard]

Created attachment 448392 [details]
Patch

[Jonathan Bedard]

Created attachment 448395 [details]
Patch

[Jonathan Bedard]

PR: https://github.com/WebKit/WebKit/pull/63

[Aakash Jain]

I think another/better way to describe this change is: Always invoke CleanGitRepo before CheckOutSource.

anyways, do we have any evidence that we need this? If I remember correctly, we added CleanGitRepo step for commit-queue when commit-queue was having issues after moving to github, but there wasn't evidence that this helped, we eventually switched commit-queue back to git.webkit.org to fix the issue. So, I don't have concrete data if this step actually helped.

[Jonathan Bedard]

(In reply to Aakash Jain from comment #7)
> I think another/better way to describe this change is: Always invoke
> CleanGitRepo before CheckOutSource.
> 
> anyways, do we have any evidence that we need this? If I remember correctly,
> we added CleanGitRepo step for commit-queue when commit-queue was having
> issues after moving to github, but there wasn't evidence that this helped,
> we eventually switched commit-queue back to git.webkit.org to fix the issue.
> So, I don't have concrete data if this step actually helped.

We're going to need it once we start touching remotes. One of the big things that step does is force us back to remote=origin and branch=main. Testing pull-requests involves changing your remote and branch. We could also look at renaming it "ResetGitRepo"

[Jonathan Bedard]

Created attachment 448452 [details]
Patch

[Aakash Jain]

Comment on attachment 448452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448452&action=review

> Tools/CISupport/ews-build/factories.py:51
> +        self.addStep(CleanGitRepo(default_branch='main'))

probably no need to pass default_branch in all these classes (except CommitQueueFactory) as 'main' is already the default in that class and we probably don't intend to change that in near future.

> Tools/CISupport/ews-build/steps.py:-3543
> -    # This somewhat quirky sequence of steps seems to clear up all the broken

Let's keep this comment unless there is specific reason to remove it.

> Tools/CISupport/ews-build/steps.py:3548
> +        self.command_list = [

Small detail to remember: __init__ might be called multiple times. Although this code seems to be safe from that perspective. 
You might consider moving the self.command_list portion to the run method(). That will simplify the logic for reading/using branch property as well.

> Tools/CISupport/ews-build/steps.py:3557
> +        branch = self.getProperty('basename', self.default_branch)

let's call it branch_name instead of basename, unless there is a specific reason for this naming. 
Also, who will be setting it? Might consider adding it in subsequent patch when it would actually be set, but it's fine to do it here as well if you have specific use in mind, and it's just a matter of splitting code in different patches.

> Tools/CISupport/ews-build/steps.py:3561
> +                command=[arg.format(branch) if '{}' in arg else arg for arg in command],

can we simplify this, looks unnecessarily complicated?

> Tools/CISupport/ews-build/steps_unittest.py:4337
>              + ExpectShell.log('stdio', stdout=''),

Nit: let's change the buildername property in this testcase to some other queue, since Commit-Queue doesn't use main currently.

> Tools/CISupport/ews-build/steps_unittest.py:4338
>              ExpectShell(command=['git', 'fetch', 'origin'], workdir='wkdir', timeout=1200, logEnviron=False) + 0

Do we need to retain 20 minutes (1200s) timeout, or can/should we reduce it?

> Tools/CISupport/ews-build/steps_unittest.py:4369
> +    def test_master(self):

Nit: let's rename it to test_success_master, and move it next to test_success.

[Jonathan Bedard]

Created attachment 448765 [details]
Patch

[Jonathan Bedard]

Created attachment 448766 [details]
Patch

[Jonathan Bedard]

Created attachment 448771 [details]
Patch

[Aakash Jain]

Comment on attachment 448771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448771&action=review

Overall looks good. My questions/points about branch_name instead of basename, and keeping the comment, seems un-answered though. 
Might be good idea to try it out on test instance or locally before deploying.

> Tools/CISupport/ews-build/steps.py:3578
> +    def __init__(self, default_branch='main', remote='origin', **kwargs):

Do we foresee any situation in which we might need a remote different than 'origin'?
If not, we can skip this parameter.

[Jonathan Bedard]

Comment on attachment 448452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448452&action=review

>> Tools/CISupport/ews-build/steps.py:-3543
>> -    # This somewhat quirky sequence of steps seems to clear up all the broken
> 
> Let's keep this comment unless there is specific reason to remove it.

I removed it because it's not really a "quirky" sequence, it's basically an explicit removal of all uncommitted code and then a hard-reset of the tracked branch. I would prefer to add comments to each line explaining what they do.

>> Tools/CISupport/ews-build/steps.py:3548
>> +        self.command_list = [
> 
> Small detail to remember: __init__ might be called multiple times. Although this code seems to be safe from that perspective. 
> You might consider moving the self.command_list portion to the run method(). That will simplify the logic for reading/using branch property as well.

Multiple times on the same object? That seems wrong, init should only be run once per object, no? In any case, moving this to run fixes another one of your comments, so doing that.

>> Tools/CISupport/ews-build/steps.py:3557
>> +        branch = self.getProperty('basename', self.default_branch)
> 
> let's call it branch_name instead of basename, unless there is a specific reason for this naming. 
> Also, who will be setting it? Might consider adding it in subsequent patch when it would actually be set, but it's fine to do it here as well if you have specific use in mind, and it's just a matter of splitting code in different patches.

We don't get to pick this one, it's coming from our pull-request hook. It's also the standard term used for "the branch my pull request to coming from" in both bitbucket and GitHub.

>> Tools/CISupport/ews-build/steps_unittest.py:4338
>>              ExpectShell(command=['git', 'fetch', 'origin'], workdir='wkdir', timeout=1200, logEnviron=False) + 0
> 
> Do we need to retain 20 minutes (1200s) timeout, or can/should we reduce it?

Oh yikes, and that's 20 minutes per command too.

I'm going to reduce it to 5. That's still probably too much, but I'm a little wary to go much lower since we're increase the number of places we're running this.

[Jonathan Bedard]

Comment on attachment 448452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448452&action=review

>> Tools/CISupport/ews-build/steps.py:-3543
>> -    # This somewhat quirky sequence of steps seems to clear up all the broken
> 
> Let's keep this comment unless there is specific reason to remove it.

I removed it because it's not really a "quirky" sequence, it's basically an explicit removal of all uncommitted code and then a hard-reset of the tracked branch. I would prefer to add comments to each line explaining what they do.

>> Tools/CISupport/ews-build/steps.py:3548
>> +        self.command_list = [
> 
> Small detail to remember: __init__ might be called multiple times. Although this code seems to be safe from that perspective. 
> You might consider moving the self.command_list portion to the run method(). That will simplify the logic for reading/using branch property as well.

Multiple times on the same object? That seems wrong, init should only be run once per object, no? In any case, moving this to run fixes another one of your comments, so doing that.

>> Tools/CISupport/ews-build/steps.py:3557
>> +        branch = self.getProperty('basename', self.default_branch)
> 
> let's call it branch_name instead of basename, unless there is a specific reason for this naming. 
> Also, who will be setting it? Might consider adding it in subsequent patch when it would actually be set, but it's fine to do it here as well if you have specific use in mind, and it's just a matter of splitting code in different patches.

We don't get to pick this one, it's coming from our pull-request hook. It's also the standard term used for "the branch my pull request to coming from" in both bitbucket and GitHub.

>> Tools/CISupport/ews-build/steps_unittest.py:4338
>>              ExpectShell(command=['git', 'fetch', 'origin'], workdir='wkdir', timeout=1200, logEnviron=False) + 0
> 
> Do we need to retain 20 minutes (1200s) timeout, or can/should we reduce it?

Oh yikes, and that's 20 minutes per command too.

I'm going to reduce it to 5. That's still probably too much, but I'm a little wary to go much lower since we're increase the number of places we're running this.

[Jonathan Bedard]

Landed 245902@main (r287857)