Eliminate redundant/out-of-sync version from BeamModulePlugin.groovy #28750

kennknowles · 2023-09-29T21:37:38Z

This addresses the initial complaint in #21302.

It does not fully "fix" it because Python and Go still track the version separately.

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 or the workflows README to see a list of phrases to trigger workflows.

kennknowles · 2023-09-29T22:03:39Z

R: @bvolpato @damccorm

github-actions · 2023-09-29T22:05:43Z

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

codecov · 2023-09-29T22:16:37Z

Codecov Report

Merging #28750 (10782ff) into master (161cd6b) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #28750   +/-   ##
=======================================
  Coverage   72.22%   72.22%           
=======================================
  Files         684      684           
  Lines      101226   101230    +4     
=======================================
+ Hits        73106    73110    +4     
  Misses      26545    26545           
  Partials     1575     1575

Flag	Coverage Δ
go	`53.41% <ø> (ø)`
python	`82.76% <ø> (+<0.01%)`	⬆️

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

see 4 files with indirect coverage changes

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

kennknowles · 2023-09-29T23:31:29Z

run java precommit

kennknowles · 2023-09-29T23:34:59Z

Confirmed that ./gradlew :release:go-licenses:java:docker succeeded locally. I'm not entirely sure that it should be running as part of the "Java PreCommit" which should probably be broken up into "Java SDK Unit Test" and the other things that run, all of which should be hermetic as possible. Pulling docker containers and licenses doesn't fit.

damccorm · 2023-10-02T13:12:21Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

@@ -429,7 +429,7 @@ class BeamModulePlugin implements Plugin<Project> {
  }

  def isRelease(Project project) {
-    return parseBooleanProperty(project, 'isRelease');
+    return !project.version.endsWith('-SNAPSHOT')


Doesn't this remove our ability to run non-release jobs against the RC commit before we're ready to publish the release?

I don't actually understand the question. Can you restate it somehow? I don't understand what these things are:

"non-release jobs"

"against the RC commit"

"the RC commit" - the commit that we tag for a release candidate. E.g. https://github.com/apache/beam/tree/v2.51.0-RC1

"non-release jobs" - any job that was previously expecting the version to end in -SNAPSHOT or has other expectations depending on the isRelease variable. This expectation is baked in all over the place FWIW - https://github.com/search?q=repo%3Aapache%2Fbeam+isRelease+language%3AGradle&type=code&l=Gradle

Oh yes, I'm aware of it being baked in all over the place, that's why I left it. Builds at the RC commit should always have -PisRelease or they won't do what we need to do with the RC.

If we have a job that expects the version to end in -SNAPSHOT that job is probably broken.

I do need to fix the ones that are directly checking hasProperty instead of calling the method though.

Eventually we should be eliminating this concept, which should probably not exist.

Converted to assigning a boolean, deliberately causing any dependencies on the prior function to break so we can catch them in review.

kennknowles · 2023-10-04T23:07:30Z

Ah, I got confused by job which I was thinking in terms of GitHub Actions or Dataflow or something. Yes, Gradle tasks that change their behavior on this flag are a nuisance.

Reviewing the usage, it has these different purposes which are all included in a release but none require a special isRelease boolean:

Signing the artifacts: if a signing key is provided, that would be enough. We probably do want to sign by default for non-SNAPSHOT versions but have a -PskipSigning property, so the result of this PR is right.
Controlling whether docker images are pushed. This should just be an independent task.
Extensive setup of the maven publish plugin. Highly suspect area of our gradle, but leaving it alone.
Hardcoding list of released architectures. Probably similar protocol to signing is appropriate.
Whether the metadata indicates the snapshot dist maven repo. This is actually a good use, conceptually, but should directly depend on whether we are issuing a command to publish to the snapshot repo.

To be clear I'm mostly intending to leave the functionality at status quo, except cleaning up an extraneous flag and an incorrect version manipulation that broke the build of 2.51.0 RC1.

I see builds are breaking so I'll iterate on this when I can get back to it, but signing off today.

kennknowles · 2023-10-05T01:24:28Z

sdks/java/container/common.gradle

@@ -107,7 +107,7 @@ task validateJavaHome {
 }

 def useBuildx = project.containerPlatforms() != [project.nativeArchitecture()]
-def pushContainers = project.rootProject.hasProperty(["isRelease"]) || project.rootProject.hasProperty("push-containers")
+def pushContainers = project.rootProject.isRelease || project.rootProject.hasProperty("push-containers")


I am honestly baffled as to why this causes the error

sdks/java/container/common.gradle': 112: Unexpected input: '{' @ line 112, column 8. docker { ^ 1 error

kennknowles · 2023-10-05T10:43:26Z

Having slept on it, I realized a couple things:

The isRelease flag probably dates back to an obsolete time were we tried to build a non-SNAPSHOT / non-dev version from a commit that was at a -SNAPSHOT / .dev version, by dynamically changing it, and then its use spread.
This is actually a few different rabbit holes and I only want to fix the version issue that interfered with the release, and handle the other problems separately.

I'll just make a new PR since that'll be nearly trivial.

github-actions bot added the build label Sep 29, 2023

kennknowles force-pushed the no-duplicate-SNAPSHOT branch 3 times, most recently from f38eff9 to 55bd323 Compare September 29, 2023 21:50

damccorm reviewed Oct 2, 2023

View reviewed changes

github-actions bot added python java go docker runners spark flink typescript labels Oct 4, 2023

Eliminate redundant/out-of-sync version from BeamModulePlugin.groovy

10782ff

kennknowles force-pushed the no-duplicate-SNAPSHOT branch from 62f581c to 10782ff Compare October 5, 2023 01:19

kennknowles commented Oct 5, 2023

View reviewed changes

kennknowles closed this Oct 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate redundant/out-of-sync version from BeamModulePlugin.groovy #28750

Eliminate redundant/out-of-sync version from BeamModulePlugin.groovy #28750

kennknowles commented Sep 29, 2023

kennknowles commented Sep 29, 2023

github-actions bot commented Sep 29, 2023

codecov bot commented Sep 29, 2023 •

edited

Loading

kennknowles commented Sep 29, 2023

kennknowles commented Sep 29, 2023

damccorm Oct 2, 2023

kennknowles Oct 4, 2023

damccorm Oct 4, 2023

kennknowles Oct 4, 2023

kennknowles Oct 4, 2023

kennknowles Oct 4, 2023

kennknowles commented Oct 4, 2023

kennknowles Oct 5, 2023

kennknowles commented Oct 5, 2023

Eliminate redundant/out-of-sync version from BeamModulePlugin.groovy #28750

Eliminate redundant/out-of-sync version from BeamModulePlugin.groovy #28750

Conversation

kennknowles commented Sep 29, 2023

GitHub Actions Tests Status (on master branch)

kennknowles commented Sep 29, 2023

github-actions bot commented Sep 29, 2023

codecov bot commented Sep 29, 2023 • edited Loading

Codecov Report

kennknowles commented Sep 29, 2023

kennknowles commented Sep 29, 2023

damccorm Oct 2, 2023

Choose a reason for hiding this comment

kennknowles Oct 4, 2023

Choose a reason for hiding this comment

damccorm Oct 4, 2023

Choose a reason for hiding this comment

kennknowles Oct 4, 2023

Choose a reason for hiding this comment

kennknowles Oct 4, 2023

Choose a reason for hiding this comment

kennknowles Oct 4, 2023

Choose a reason for hiding this comment

kennknowles commented Oct 4, 2023

kennknowles Oct 5, 2023

Choose a reason for hiding this comment

kennknowles commented Oct 5, 2023

codecov bot commented Sep 29, 2023 •

edited

Loading