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

Clarify instructions to fetch all upstream branches #1266

Merged
merged 2 commits into from Oct 1, 2020

Conversation

kcmartin
Copy link
Contributor

@kcmartin kcmartin commented Oct 1, 2020

Clarify instructions to state fetch all branches from upstream website repo, not just master branch

Clarify that one needs to fetch all branches from upstream website repo, not just master branch
@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 1, 2020
@k8s-ci-robot k8s-ci-robot added area/release-team Issues or PRs related to the release-team subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels Oct 1, 2020
@kcmartin
Copy link
Contributor Author

kcmartin commented Oct 1, 2020

/assign @annajung

@@ -352,7 +352,7 @@ To merge `master` into `dev-[future release]` on your local fork:
# Step 0 (if you don't already have a remote called "upstream")
git remote add upstream https://github.com/kubernetes/website.git
# Step 1
git fetch upstream master
git fetch upstream #grab all branches from upstream repo
Copy link
Member

Choose a reason for hiding this comment

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

Since we are pulling the changes from only master branch to keep dev-[future release] in sync, I guess fetching just master branch would suffice here. It avoids fetching all other branches and various changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

We actually need to pull both master and dev-[future release], so I don't think we want to pull all branches but add a step to fetch dev-[future release].

@savitharaghunathan @kcmartin What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi-- the edit was prompted by an error I got following the steps in the role handbook:

git checkout --track upstream/dev-1.20 fatal: 'upstream/dev-1.20' is not a commit and a branch 'dev-1.20' cannot be created from it

After I used git fetch upstream, to fetch all the branches, I was able to complete the steps, hence the edit. I'm open to @annajung 's suggestion, or whatever makes sense, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can fetch master and dev-[future-release] in just one step?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a great idea. I'm not sure if git fetch allows multiple branches? We should take a look and see! :)

But, I don't think there's anything wrong with adding an additional command git fetch upstream dev-[future release] below master one. Or we can also do git fetch upstream master && git fetch upstream dev-[future release] to make it one-liner if git fetch doesn't allow multiple branches.

Let's follow up on this and see if we can find anything, else I think adding extra command would be sufficient! 😀

Copy link
Member

Choose a reason for hiding this comment

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

Hi @kcmartin, Thank you for adding the error message, now I remember going through that and had to fetch dev-branch. I used to alternate between fetching everything and just fetching what is required. My apologies for the review comment. I see no harm now in adding an additional step or merging @kcmartin's suggestion.

@LappleApple LappleApple added this to In progress in Release Team Oct 1, 2020
@LappleApple LappleApple moved this from In progress to In Review in Release Team Oct 1, 2020
Per PR discussion we don't really need to fetch all branches
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 1, 2020
@kcmartin
Copy link
Contributor Author

kcmartin commented Oct 1, 2020

What do you think @annajung and @savitharaghunathan ?
I added the additional step for fetching the dev branch.
Thank you both so much for the thoughtful review comments!

@annajung
Copy link
Contributor

annajung commented Oct 1, 2020

/lgtm

I'll leave approval to @savitharaghunathan

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2020
Copy link
Member

@savitharaghunathan savitharaghunathan left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: annajung, kcmartin, savitharaghunathan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3fc9868 into kubernetes:master Oct 1, 2020
Release Team automation moved this from In Review to Done (1.20) Oct 1, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Oct 1, 2020
@kcmartin kcmartin deleted the patch-1 branch October 1, 2020 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release-team Issues or PRs related to the release-team subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority sig/release Categorizes an issue or PR as relevant to SIG Release. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants