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

docs: add forking instructions + workflow + fix contributing notes #16025

Merged
merged 2 commits into from May 6, 2021

Conversation

nbusseneau
Copy link
Member

See individual commits.

@nbusseneau nbusseneau added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. sig/contributing Impacts contribution workflow, guidelines, and tools. labels May 5, 2021
@nbusseneau nbusseneau requested review from pchaigno and aanm May 5, 2021 14:24
@nbusseneau nbusseneau requested review from a team as code owners May 5, 2021 14:24
@nbusseneau nbusseneau requested a review from qmonnet May 5, 2021 14:24
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

(Looks good to me from the docs-structure point of view.)

Working from a fork will make it harder for contributors to rebase their PRs, no? Would that make sense instead to work on cilium/cilium but to add the fork as a distinct remote for pushing the changes?

@nbusseneau
Copy link
Member Author

nbusseneau commented May 5, 2021

Working from a fork will make it harder for contributors to rebase their PRs, no? Would that make sense instead to work on cilium/cilium but to add the fork as a distinct remote for pushing the changes?

Can you clarify which rebase scenario you think is harder? 🤔

As for using cilium/cilium and having the fork as a separate remote: I think this is a bad idea.
Not in the sense that you should not do it (if you do and it suits you, please continue :D), but in the sense that this is not standard practice. The usual way for people to work with forks/upstream on GitHub is to fork, then clone the fork, then setup the upstream as a remote (see doc).
I personally think the documentation is written specifically for not-so-Git-savvy peeps since Git aficionados will do whatever works best for them anyway no matter the documentation... hence my thinking that we should try to keep it standard ^^

(As an aside note, I was under the impression the thing I've just documented is what we actually were supposed to be doing for quite some time now :D)

Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

As for using cilium/cilium and having the fork as a separate remote: I think this is a bad idea.

Re-reading your PR, I'm not sure what I had in mind. Agreed that cloning the fork and adding cilium/cilium as upstream remote is the same thing as cloning the latter and adding the fork as a remote. I probably got confused somewhere on my first review, apologies.

Looks good, couple of nits.

Should help ensure Git newcomers properly manage their forks w.r.t. to
the upstream remote.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

- Several notes in the contributing guide were not displayed on the
  page due to a missing semi-colon.
- Moved the note about `git commit -s` to its proper place.
- Also standardized whitespace around sub-items in "Making changes".

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 6, 2021
@nbusseneau
Copy link
Member Author

Sneaky push to fix commit logs... (no #topys in there, I swear, don't need to look 😇)

@ti-mo
Copy link
Contributor

ti-mo commented May 6, 2021

(no #topys in there, I swear, don't need to look 😇)

image

@ti-mo ti-mo merged commit 044de92 into cilium:master May 6, 2021
@nbusseneau nbusseneau deleted the pr/fork-documentation branch May 10, 2021 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/contributing Impacts contribution workflow, guidelines, and tools.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants