Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEAM-8956] Begin unifying contributor instructions into a single location #10366

Closed
wants to merge 10 commits into from
Closed

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Dec 12, 2019

We have overlapping and sometimes contradictory docs on how to setup and build BEAM in four different places I've found:

README.md
CONTRIBUTING.md
https://cwiki.apache.org/confluence/display/BEAM/Contributor+FAQ
https://beam.apache.org/contribute/

We should probably pick one as the source of truth and rewrite the
other three to simply point to it. I propose putting all checkout,
build, test, commit, and push instructions in CONTRIBUTING.md in the
repo.

@iemejia

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.

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

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
Build Status
Python Build Status
Build Status
Build Status
Build Status
--- Build Status
Build Status
Build Status
Build Status
--- --- Build Status
XLang --- --- --- Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status 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.

@elharo elharo changed the title Begin unifying contributor instructions into a single location [BEAM-8956] Begin unifying contributor instructions into a single location Dec 12, 2019
@elharo
Copy link
Contributor Author

elharo commented Dec 12, 2019

R: @iemejia

CONTRIBUTING.md Outdated
* Python 3
* pip
* setuptools
* Go 1.12 or later
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you group the tools by programming languages (Java, Python, Go)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

CONTRIBUTING.md Outdated
you need additional tools installed in your system including:

* JDK 8 or later
* Python 3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you clarify which Python 3? (3.4, 3.7, etc.?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea which versions are supported/required?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python 3.5, 3.6, and 3.7.

suztomo-macbookpro44:beam suztomo$ find . -name 'py33'
suztomo-macbookpro44:beam suztomo$ find . -name 'py34'
suztomo-macbookpro44:beam suztomo$ find . -name 'py35'
./sdks/python/container/py35
./sdks/python/test-suites/dataflow/py35
./sdks/python/test-suites/tox/py35
./sdks/python/test-suites/direct/py35
./sdks/python/test-suites/portable/py35
suztomo-macbookpro44:beam suztomo$ find . -name 'py36'
./sdks/python/container/py36
./sdks/python/test-suites/dataflow/py36
./sdks/python/test-suites/tox/py36
./sdks/python/test-suites/direct/py36
./sdks/python/test-suites/portable/py36
suztomo-macbookpro44:beam suztomo$ find . -name 'py37'
./sdks/python/container/py37
./sdks/python/test-suites/dataflow/py37
./sdks/python/test-suites/tox/py37
./sdks/python/test-suites/direct/py37
./sdks/python/test-suites/portable/py37
suztomo-macbookpro44:beam suztomo$ find . -name 'py38'
suztomo-macbookpro44:beam suztomo$ 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

README.md Outdated
@@ -106,7 +94,8 @@ To get involved in Apache Beam:
* [Subscribe](mailto:[email protected]) or [mail](mailto:[email protected]) the [[email protected]](http://mail-archives.apache.org/mod_mbox/beam-dev/) list.
* Report issues on [JIRA](https://issues.apache.org/jira/browse/BEAM).

We also have a [contributor's guide](https://beam.apache.org/contribute/contribution-guide/).
Instructions for building and testing Beam itself
are in the [contributor's guide](CONTRIBUTING.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is going to name CONTRIBUTING.md as “contributor’s guide”, which conflicts with the page in the wiki. How about just naming “CONTRIBUTING.md”?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One or the other of these is going away. I'm not sure which yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kenn responded to the email thread to have CONTRIBUTING.md link to https://beam.apache.org/contribute/contribution-guide/

CONTRIBUTING.md Outdated
python-pip \
virtualenv \
tox \
docker-ce
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is Go 1.12?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go can't be installed by apt-get. Added pointer to instructions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

CONTRIBUTING.md Outdated
* tox
* Docker

To install these in a Debian-based distribution:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if you can specify debian release version you used, such as Debian 9 Stretch. The availability of package differ based on the version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should work with any reasonably recent debian. I'm going to assume that until told otherwise.

CONTRIBUTING.md Outdated
@@ -27,3 +27,31 @@ for details, such as:
* Development setup and testing your changes
* Submitting a pull request and finding a reviewer

To build and install the whole project from the source distribution,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per dev@ thread, let's move all concrete technical instructions to the linked contribution guide.

Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL. Now unified into website.

FYI, I have not yet figured out how to build/run the website locally. Something about my gLinux docker setup is off:

Task :website:buildDockerImage FAILED
Got permission denied while trying to connect to the Docker daemon socket at unix:///var/run/docker.sock: Post http://%2Fvar%2Frun%2Fdocker.sock/v1.40/build?buildargs=%7B%7D&cachefrom=%5B%5D&cgroupparent=&cpuperiod=0&cpuquota=0&cpusetcpus=&cpusetmems=&cpushares=0&dockerfile=Dockerfile&labels=%7B%7D&memory=0&memswap=0&networkmode=default&rm=1&session=zsx1afhzf5weazb0n9n456xgb&shmsize=0&t=beam-website&target=&ulimits=null&version=1: dial unix /var/run/docker.sock: connect: permission denied

@udim
Copy link
Member

udim commented Dec 19, 2019

PTAL. Now unified into website.

FYI, I have not yet figured out how to build/run the website locally. Something about my gLinux docker setup is off:

Task :website:buildDockerImage FAILED
Got permission denied while trying to connect to the Docker daemon socket at unix:///var/run/docker.sock: Post http://%2Fvar%2Frun%2Fdocker.sock/v1.40/build?buildargs=%7B%7D&cachefrom=%5B%5D&cgroupparent=&cpuperiod=0&cpuquota=0&cpusetcpus=&cpusetmems=&cpushares=0&dockerfile=Dockerfile&labels=%7B%7D&memory=0&memswap=0&networkmode=default&rm=1&session=zsx1afhzf5weazb0n9n456xgb&shmsize=0&t=beam-website&target=&ulimits=null&version=1: dial unix /var/run/docker.sock: connect: permission denied

Seems like you need to configure Docker in "sudoless" mode, i.e., give your user access to that socket file.

Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL

Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping. Can we merge this? This isn't completely done yet, but I think it's a big step forward from where the docs are now.


```
$ export GOPATH=`pwd`/sdks/go/examples/.gogradle/project_gopath
$ go get github.com/linkedin/goavro
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for picking up my comment.

Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping. Can we move this forward?

@aaltay
Copy link
Member

aaltay commented Jan 10, 2020

@iemejia @kennknowles could you please review this PR?

iemejia added a commit that referenced this pull request Jan 13, 2020
@iemejia
Copy link
Member

iemejia commented Jan 13, 2020

Done, merged manually. Thanks for the reminder @aaltay and sorry @elharo for taking so long on merging this. Thanks for taking care of this.

@iemejia iemejia closed this Jan 13, 2020
@kennknowles
Copy link
Member

Ah, sorry. I somehow failed to send my comment, which was that we should keep CONTRIBUTING.md just to have a pointer to the primary instructions. This file is treated specially by GitHub. It is not terribly important. I don't know the real impact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants