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

Update source.md - Default to stable branch #5207

Merged
merged 3 commits into from
Mar 16, 2023

Conversation

coincashew
Copy link
Contributor

@coincashew coincashew commented Feb 27, 2023

Motivation

Safer: To make the default git cloning behavior for "build from source" production safe, thus avoiding any misunderstandings.
Common default branch: All other consensus clients github repos default to their respective stable branch.

Description

Adds stable branch flag to git clone operation.

@coincashew coincashew requested a review from a team as a code owner February 27, 2023 07:16
@CLAassistant
Copy link

CLAassistant commented Feb 27, 2023

CLA assistant check
All committers have signed the CLA.

twoeths
twoeths previously approved these changes Feb 27, 2023
@nflaig
Copy link
Member

nflaig commented Feb 27, 2023

@coincashew Thanks for updating this but we can not merge changes directly to stable branch as those need to be included to unstable first and then will be merged from there.

We actually did a update to the source installation docs a while ago but for it to update the actual docs page it needs to be in stable but it currently just is in unstable, see PR #5056.

I opted for adding a note instead of directly specifying which branch should be cloned as I think if you run in prod it might even be best to specify a tag aka version instead of a branch.

dapplion
dapplion previously approved these changes Feb 27, 2023
@dapplion
Copy link
Contributor

@nflaig users don't read notes they just copy commands (me included!) I think adding the stable branch as a safe default is good and additive to your note

@dapplion dapplion enabled auto-merge (squash) February 27, 2023 10:46
@nflaig
Copy link
Member

nflaig commented Feb 27, 2023

just need to adapt the note then as it currently says that unstable will be pulled by default, we have a bit of a conflict here since the note was added on unstable branch whereas this PR targets the stable branch

@nflaig
Copy link
Member

nflaig commented Feb 27, 2023

Common default branch: All other consensus clients github repos default to their respective stable branch.

@coincashew looks like prysm does not do this either, they default to develop, in their docs they reference the master branch though. In your guide it points to master as well so should be fine

@nflaig nflaig disabled auto-merge February 27, 2023 11:03
Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

@coincashew This is definitely a good update to the docs and I think as @dapplion mentioned we should just add the -b stable by default. Need to reopen this against the unstable branch and do the docs updates there to avoid merge conflicts when merging unstable to stable.

@wemeetagain wemeetagain changed the base branch from stable to unstable February 28, 2023 13:30
@wemeetagain wemeetagain dismissed stale reviews from dapplion and twoeths February 28, 2023 13:30

The base branch was changed.

wemeetagain
wemeetagain previously approved these changes Mar 1, 2023
Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

we also need to update the warning or just remove it entirely

!!! warning
`git clone` will check out the default `unstable` branch. If you are running Lodestar in production, we recommend to either use the `stable` branch
by running `git switch stable` or to use a specific version by running `git checkout <version>`, e.g. `git checkout v1.3.0`.

Removed warning, as requested by nflaig
@coincashew coincashew changed the title Update source.md Update source.md - Default to stable branch Mar 13, 2023
@coincashew
Copy link
Contributor Author

Requested change made. Good feedback all around. Thanks.

Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the update @coincashew

@nflaig nflaig merged commit 3d937ee into ChainSafe:unstable Mar 16, 2023
@coincashew coincashew deleted the stable branch March 20, 2023 04:24
@wemeetagain
Copy link
Member

🎉 This PR is included in v1.7.0 🎉

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