Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

documentation: Adapt CONTRIBUTING.md for the move to gitlab #1604

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Aug 14, 2020

This is an attempt at keeping the docs up to date with the move from github to gitlab.
Since this document will be used as a template for prplWrt as well, it's double important that it stays up to date.

I anticipate we'll need another update once the move is complete and we've settled on how to work effectively with gitlab a bit more.
The "Merge request workflow" in particular might change significantly, but I don't want to do it yet because I'm not certain yet what it will look like.
Also, I don't know how (if) the DCO check still works with gitlab, or if we need to tweak things a little there.
No doubt we'll discover other issues.

So, this is just an initial version of the contribution guidelines, adapted for gitlab.

See PPM-335

@ghost ghost requested a review from arnout August 14, 2020 21:19
@ghost ghost self-assigned this Aug 14, 2020
This is an attempt at keeping the docs up to date with the move from github to gitlab.
Since this document will be used as a template for prplWrt as well, it's double important that it stays up to date.

I anticipate we'll need another update once the move is complete and we've settled on how to work effectively with gitlab a bit more.
The "Merge request workflow" in particular might change significantly, but I don't want to do it yet because I'm not certain yet what it will look like.
Also, I don't know how (if) the DCO check still works with gitlab, or if we need to tweak things a little there.
No doubt we'll discover other issues.

So, this is just an *initial version* of the contribution guidelines, adapted for gitlab.

See PPM-335

Signed-off-by: Frederik Van Bogaert <[email protected]>
@ghost ghost force-pushed the feature/ppm-335-gitlab-move-documentation-update branch from 30a6e89 to ffbf2aa Compare August 14, 2020 21:20
@ghost
Copy link
Author

ghost commented Aug 14, 2020

Before anyone comments: yes, there's some fixes related to JIRA and Confluence as well in here.

I didn't feel the need to do it in a separate PR or commit, since these are just small documentation fixes.

@ghost ghost added the don't merge This PR is not ready for merge or review label Aug 14, 2020
Copy link
Collaborator

@adam1985d adam1985d left a comment

Choose a reason for hiding this comment

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

gave a few comments but nothing is a blocker :-)

@@ -4,13 +4,14 @@ You can contribute to prplMesh in various ways: reporting bugs, improving docume

## Getting involved

A large part of the conversation takes place directly on github, through issues, pull requests, and comments on commits.
Thus, it is advisable to subscribe to (i.e. watch) the issues you are interested in, or the project as a whole.
Part of the conversation takes place directly on gitlab through merge requests, and comments on commits.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Part of the conversation takes place directly on gitlab through merge requests, and comments on commits.
Part of the conversation takes place directly on gitlab through merge requests and comments on commits.


Daily conversations take place on [Slack](https://prplfoundation.slack.com/).
You can ask for an invite to anyone currently involved, e.g. by sending a message to an active contributer on github.
You can ask for an invite to anyone currently involved, e.g. by sending a message to an active contributer on gitlab.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
You can ask for an invite to anyone currently involved, e.g. by sending a message to an active contributer on gitlab.
You can ask for an invite to anyone currently involved, e.g. by sending a message to an active contributor on gitlab.

@@ -187,16 +188,18 @@ This is a short way for you to say that you are entitled to contribute the patch
It is a legal statement that asserts your agreement with the [DCO](#developers-certificate-of-origin).
Therefore, it must have your *real name* (First Last) and a valid e-mail address.
Adding this tag can be done automatically by using `git commit -s`.
If you are editing files and committing through GitHub, you must write your real name in the “Name” field in GitHub settings and the email used in the "Signed-off-by:" must be your primary github email.
You must manually add the "Signed-off-by:" to the commit message that github requests.
If you are editing files and committing through GitLab, you must write your real name in the “Full Name” field in your GitLab profile and the email used in the "Signed-off-by:" must be your "Commit email" address.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this originally was Github and thus you changed to Gitlab, but all other places refer to it as lower-case gitlab. better stick to one (lower/upper case the G for all occurrences)

Suggested change
If you are editing files and committing through GitLab, you must write your real name in the “Full Name” field in your GitLab profile and the email used in the "Signed-off-by:" must be your "Commit email" address.
If you are editing files and committing through GitLab, you must write your real name in the “Full Name” field in your GitLab profile, and the email used in the "Signed-off-by:" must be your "Commit email" address.

If you are editing files and committing on your local PC, set your name and email with:

```bash
git config --global user.name "my name"
git config --global user.email "[email protected]"
```

### Pull Request workflow
Then, adding the "Signed-off-by" line is as simple as using `git commit -s` instead of `git commit` (using an alias is recommended, e.g. `git config --global alias.ci='commit -s'`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Then, adding the "Signed-off-by" line is as simple as using `git commit -s` instead of `git commit` (using an alias is recommended, e.g. `git config --global alias.ci='commit -s'`)
Then, adding the "Signed-off-by" line is as simple as using `git commit -s` instead of `git commit` (using an alias is recommended. E.g. `git config --global alias.ci='commit -s'`)

Comment on lines 217 to 221
7. Author addresses review comments in additional fixup commits.
* If no more fixes are needed
* Author rebases with `git rebase -i --autosquash master` to clean up the pull request.
* Author rebases with `git rebase -i --autosquash master` to clean up the merge request.
* If more fixes are needed (suggested by reviewers or by the author himself)
* Author does rebase-force-push to squash fixup commits and asks for a followup review (this makes the next review iterations simpler)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is how we originally stated the flow to be - but I am not sure the fixup-commits after the review are really used (I didn't encounter them if the review process for a long time now.
Usually, the flow I see is:
review comments are addressed in one of the 3 ways:

  1. decision not to accept the comment (and no change is done according to that comment)
  2. accept the comment requested change, and if it is a simple fix - resolve the comment in the conversation (but fix is already squashed to the commit that introduced the change). If the fix is not simple the conversation isn't resolved.
  3. comment required more discussion and then a ping-pong is started until either '1' or '2' above is decided.

either way didn't see a PR that the changes were introduced as fixup, If this is the flow we want to use - we need to enforce it for future PR/merge requests.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
don't merge This PR is not ready for merge or review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant