Skip to content

[BEAM-5379] Add go modules#9085

Closed
zachbadgett wants to merge 2 commits intoapache:masterfrom
zachbadgett:go-sdk-modules
Closed

[BEAM-5379] Add go modules#9085
zachbadgett wants to merge 2 commits intoapache:masterfrom
zachbadgett:go-sdk-modules

Conversation

@zachbadgett
Copy link

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:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Build Status
Python Build Status
Build Status
--- Build Status
Build Status
Build Status --- --- Build Status

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@zachbadgett
Copy link
Author

My take on Go modules: keep GoGradle, disable all dependency tasks; go.mod and go.sum live in the base directory so all Go related jobs can function properly.

I'll add the license to go.mod but go.sum does not support comments: https://github.com/golang/go/blob/master/src/cmd/go/internal/modfetch/fetch.go#L326

Still a few things to do related to documentation, wanted to get buy in on the implementation first.

@zachbadgett zachbadgett force-pushed the go-sdk-modules branch 5 times, most recently from f60c640 to 1c55391 Compare July 17, 2019 10:31
@zachbadgett
Copy link
Author

Ran into some issues on CI with permissions and the fact a new gopath is created for each directory, I'll have a fix pushed up for it today.

@lostluck
Copy link
Contributor

Thank you! It's very exciting someones jumping on this! It's perpetually been on my todo list, but I've always lacked the time to get the Gradle expertise.

If it's possible to disable the dependency resolution tasks, then I have no problem with this approach. I hadn't even considered that as an option.

As for the RAT check, we can add exclusions for go.mod & go.sum files in the root build.gradle file. Given they're generated, this shouldn't be problematic.
https://github.com/apache/beam/blob/master/build.gradle#L77

One thing I'd like to ensure is that the SDK is not marked at the same version as mainline beam at this time. We need to tag it sdks/go/pkg/beam+v0. or similar at this time. And ideally we have two modules, one for the actual user package in sdks/go/pkg/beam and one for sdks/go
If only to make the sdks/go depending on the user package very explicit. It also makes it clear that sdks/go/test etc depend on a version of the user package (which we can then force set to a specific commit or similar for the jenkins runs)

Largely:

  • The SDK is still experimental and being v0 is a good signal for that.
  • v2 has additional implications around quality that the SDK isn't prepared for (just yet). It's not a v2+ project at this time.
  • (minor) We wouldn't need to update all the imports to refer to v2 to be in proper module compatibility.

@lostluck lostluck self-requested a review July 18, 2019 20:49
@lostluck lostluck mentioned this pull request Jul 18, 2019
3 tasks
@lostluck
Copy link
Contributor

Run Go PostCommit

@lostluck
Copy link
Contributor

Running the Post Commit makes it look like there's a bit more baking to be done WRT the integration tests, but this approach is still promising!
https://builds.apache.org/job/beam_PostCommit_Go_PR/58/console
It looks like the integration tests are doing something weird, causing some recursion...

@zachbadgett
Copy link
Author

@lostluck I have fixes and optimizations I need to push up, however I'm waiting for #9200 so we can prevent the readonly files from causing Jenkins issues.

@stale
Copy link

stale bot commented Sep 29, 2019

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

@stale stale bot added the stale label Sep 29, 2019
@stale
Copy link

stale bot commented Oct 6, 2019

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@stale stale bot closed this Oct 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants