Remove never-worked githubPush trigger

[Abacn]

A couple of test (efficiency) optimizations.

Closes#26561 ; Part of #26479 ; followup of #26491

• Increase Sickbay PostCommit trigger interval from 6h to daily

• Increase Java RunnerV2 VR interval from 6h to 8h (the streaming test already runs 7h)

Please add a meaningful description for your change here

---

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

• Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
• Update CHANGES.md with noteworthy changes.
• If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

[Abacn]

run seed job

[Abacn]

run seed job

[Abacn]

Verified the schedule change works:

[Abacn]

Run Python_Xlang_Gcp_Direct PostCommit

[Abacn]

Run Python_Xlang_Gcp_Dataflow PostCommit

[codecov]

Codecov Report

Merging #26562 (0dc5874) into master (5772bf7) will increase coverage by 0.90%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #26562 +/- ##
==========================================
+ Coverage 71.17% 72.08% +0.90% 
==========================================
Files 787 745 -42 Lines 103293 101203 -2090 ==========================================
- Hits 73522 72954 -568 + Misses 28283 26789 -1494 + Partials 1488 1460 -28 

Flag	Coverage Δ
python	81.13% <ø> (+1.26%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 92 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[Abacn]

R: @ahmedabu98

[github-actions]

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

[ahmedabu98]

LGTM, just one nit

[ahmedabu98]

instead of try catch block, can do something like

Suggested change

	String buildSchedule ='H H/6 * * *'
	try {
	buildSchedule = scope.getProperty('buildSchedule')
	} catch (MissingPropertyException ignored) {
	// do nothing
	}
	String buildSchedule = scope.hasProperty('buildSchedule')?.getProperty(scope) ?:'H H/6 * * *'

[Abacn]

This is a weird part of groovy. Tested that getProperty('buildSchedule') returns the string if previously set but hasProperty('buildSchedule') always returns false value.

refer to: https://stackoverflow.com/questions/17921490/hasproperty-returns-null (though the answer there was not super clear). Basically it seems hasProperty does not recognize the property dynamically added to the instance, but hasProperty does.

[ahmedabu98]

I see. I looked again at how buildSchedule is passed into this method, would it not be cleaner to create a new parameter in PostcommitJobBuilder.postCommitJob()?

[Abacn]

Yeah I was considering this. The signature of static postCommitJob and PostcommitJobBuilder constructor both have the last parameter is a Closure so that they can be called in a special syntax method(para1, para2) { closure } and pass the code block in the braces as its last parameter. If I need to add the parameters then I need to add buildSchedule as the second last parameter. However the method is called in both method(para1, para2) { closure } and method(para1, para2, ..., closure) and for the later I need to specify buildSchedule because it is not the last parameter.

Anyway it turns out that adding a parameter introduces more code change (and must breaks current callers) so I decided to use the existing scope

[ahmedabu98]

Makes sense, thanks for the explanation. Have you considered defining the value inside the closure? So as to be accessed with jobDefinition.buildSchedule? I just haven't seen a variable declared like this before in our repo and don't know if there's any downsides to it.

Up to you, I'll defer to your judgement here.

[Abacn]

yeah, ideally PostcommitJobBuilder should just be designed as PrecommitJobBuilder where parameters can be set as a map-like constructor (as precommit job DSLs) then I would just need to add one more parameter there. However currently PostcommitJobBuilder does not quite follow a Builder pattern. Both constructor and its static method (postCommitJob) are used for assembling postcommit jobs. Opened #26588 addressing this as a future task

[ahmedabu98]

Thanks for this change! The xlang GCP dataflow test time really did get cut in half; and direct now only takes a few minutes to run

[Abacn]

Run Python_Runners PreCommit