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

improve contribution guide #3744

Merged
merged 7 commits into from
May 6, 2020
Merged

improve contribution guide #3744

merged 7 commits into from
May 6, 2020

Conversation

scrye
Copy link
Contributor

@scrye scrye commented May 4, 2020

The previous version of the contribution guide lacked a lot of
information on how to get started with contributing to the
project. This PR adds a more step-by-step description of how
to contribute to an issue.

Also points the Github README to the new documentation site.


This change is Reviewable

The previous version of the contribution guide lacked a lot of
information on how to get started with contributing to the
project. This PR adds a more step-by-step description of how
to contribute to an issue.

Also points the Github README to the new documentation site.
@scrye scrye added the c/documentation Improvements or additions to documentation label May 4, 2020
@scrye scrye requested a review from shitz May 4, 2020 20:23
@scrye scrye self-assigned this May 4, 2020
Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @scrye and @shitz)


README.md, line 9 at r1 (raw file):

[![GitHub issues](https://img.shields.io/github/issues/scionproto/scion/help%20wanted.svg?label=help%20wanted&color=purple)](https://github.com/scionproto/scion/issues?q=is%3Aopen+is%3Aissue+label%3A%22help+wanted%22)
[![GitHub issues](https://img.shields.io/github/issues/scionproto/scion/good%20first%20issue.svg?label=good%20first%20issue&color=purple)](https://github.com/scionproto/scion/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22)
[![Release](https://img.shields.io/github/release-pre/scionproto/scion.svg)](https://github.com/scionproto/scion/releases)

Duplicate

@matzf
Copy link
Contributor

matzf commented May 5, 2020

Perhaps a good place to mention this: the link to the docs in CONTRIBUTE.md is broken.

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @scrye and @shitz)


README.md, line 15 at r1 (raw file):
Perhaps not strictly necessary for this change, but personally, I find that the README, i.e. the landing page for the project, looks a bit deserted now. Personly, Id choose to keep a bit more info in the README itself; heres a suggestion:

# SCION
> Scalability, Control and Isolation On next-generation Networks
> Implementation of a not-so-far-in-the-future Internet architecture

Welcome to the open-source implementation of [SCION](http://www.scion-architecture.net), a future Internet architecture.

... description of the SCION in ~5 sentences ...

## Getting Started

### Installing

Join [SCIONLab](https://www.scionlab.org) if you're interested in playing with SCION in an operational global test deployment of SCION. As part of the SCIONLab project, we support [pre-built binaries as Debian packages](https://docs.scionlab.org/content/install/).

### Building

Refer to the [documentation site](https://anapaya-scion.readthedocs-hosted.com/en/latest/contribute.html#setting-up-the-development-environment) for instructions on how to install build dependencies, build and run SCION.


## Contributing

Please refer to [contribute.rst](https://anapaya-scion.readthedocs-hosted.com/en/latest/contribute.html) for contribution guidelines.


## License

... license badge ...

doc/contribute.rst, line 57 at r1 (raw file):

.. _setting-up-the-development-environment:

Setting up the development environment

Maybe this could be a separate page as it's relevant for people who just want to build SCION but not contribute (yet ;)).


doc/contribute.rst, line 85 at r1 (raw file):

   .. code-block:: bash

      ./env/deps

What are these dependencies actually needed for?


doc/contribute.rst, line 94 at r1 (raw file):

#. SCION networks are composed of many different applications. To simplify testing, we provide a
   tool that generates test topologies. To generate the files required by the default topology (see
   ``doc/fig/default_topo.png`` for a diagram of this topology), run:

Maybe add a note that this is optional. (Almost) everything can be run just fine without docker.


doc/contribute.rst, line 118 at r1 (raw file):

    .. code-block:: bash

       ./scion.sh test

Maybe mention how to run lint, too?

Also, is there an easy-enough way to run integration tests locally?


doc/contribute/git.rst, line 43 at r1 (raw file):

Now that you have your own fork, follow the steps in the
`README <https://github.com/scionproto/scion/blob/master/README.md>`__ to set up

This information is now in a different place.


doc/contribute/git.rst, line 86 at r1 (raw file):

Git has many powerful features, but the one you will probably use most are
branches. You can think of branches as a different version of your project,
however, they are much more lightweight than their SVN counterparts and

Probably not too many people left who know SVN better than git 😉
To be honest, I the information here is really covered enough in the guides linked above (https://try.github.io).


doc/contribute/git.rst, line 192 at r1 (raw file):

  state.
- If you have any git problems, ping someone on the slack channel for help.
  Don't suffer alone :)
  • Run test and lint

Copy link
Contributor Author

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Done.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @matzf, @oncilla, and @shitz)


README.md, line 9 at r1 (raw file):

Previously, Oncilla wrote…

Duplicate

Done.


README.md, line 15 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Perhaps not strictly necessary for this change, but personally, I find that the README, i.e. the landing page for the project, looks a bit deserted now. Personly, Id choose to keep a bit more info in the README itself; heres a suggestion:

# SCION
> Scalability, Control and Isolation On next-generation Networks
> Implementation of a not-so-far-in-the-future Internet architecture

Welcome to the open-source implementation of [SCION](http://www.scion-architecture.net), a future Internet architecture.

... description of the SCION in ~5 sentences ...

## Getting Started

### Installing

Join [SCIONLab](https://www.scionlab.org) if you're interested in playing with SCION in an operational global test deployment of SCION. As part of the SCIONLab project, we support [pre-built binaries as Debian packages](https://docs.scionlab.org/content/install/).

### Building

Refer to the [documentation site](https://anapaya-scion.readthedocs-hosted.com/en/latest/contribute.html#setting-up-the-development-environment) for instructions on how to install build dependencies, build and run SCION.


## Contributing

Please refer to [contribute.rst](https://anapaya-scion.readthedocs-hosted.com/en/latest/contribute.html) for contribution guidelines.


## License

... license badge ...

Done, I've added your ideas to the first page. I didn't add a description of the technology yet, but we can add that one later.


doc/contribute.rst, line 57 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Maybe this could be a separate page as it's relevant for people who just want to build SCION but not contribute (yet ;)).

Done.


doc/contribute.rst, line 85 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

What are these dependencies actually needed for?

Packages needed to run python stuff locally, e.g., the topology generator.


doc/contribute.rst, line 94 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Maybe add a note that this is optional. (Almost) everything can be run just fine without docker.

Done.


doc/contribute.rst, line 118 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Maybe mention how to run lint, too?

Also, is there an easy-enough way to run integration tests locally?

Done.


doc/contribute/git.rst, line 43 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

This information is now in a different place.

Done.


doc/contribute/git.rst, line 86 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Probably not too many people left who know SVN better than git 😉
To be honest, I the information here is really covered enough in the guides linked above (https://try.github.io).

Fair enough, removed.


doc/contribute/git.rst, line 192 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…
  • Run test and lint

I removed this entire section, but added a new one for unit tests and linting.

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r2.
Reviewable status: 9 of 13 files reviewed, 8 unresolved discussions (waiting on @matzf and @shitz)

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 14 files reviewed, 9 unresolved discussions (waiting on @matzf, @scrye, and @shitz)


doc/build/setup.rst, line 54 at r3 (raw file):

      ./scion.sh run

#. To stop the infrastructure, run:

I think it would be good to add another step here:

#. To verify that your topology is setup correctly, run an end-to-end reachability test

   .. code-block:: bash
   
      ./bin/end2end_integration

Copy link
Contributor Author

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 14 files reviewed, 9 unresolved discussions (waiting on @matzf, @oncilla, and @shitz)


doc/build/setup.rst, line 54 at r3 (raw file):

Previously, Oncilla wrote…

I think it would be good to add another step here:

#. To verify that your topology is setup correctly, run an end-to-end reachability test

   .. code-block:: bash
   
      ./bin/end2end_integration

Done.

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r2, 1 of 1 files at r3, 1 of 2 files at r4.
Reviewable status: 11 of 14 files reviewed, 8 unresolved discussions (waiting on @matzf and @shitz)

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 5 files at r2, 1 of 2 files at r4.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @matzf and @shitz)

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @matzf, @scrye, and @shitz)


doc/contribute/go-style.rst, line 60 at r4 (raw file):

- ``Seg`` instead of ``Segment``
- ``Msger`` instead of ``Messenger``

that always rubs me in the wrong way. I think the correct form is msgr actually

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @matzf, @scrye, and @shitz)

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @matzf and @shitz)

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shitz)

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r6.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shitz)

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shitz)

@scrye scrye merged commit 75acff9 into scionproto:master May 6, 2020
@scrye scrye deleted the pubpr-contribute branch May 6, 2020 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants