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

Create a new branch for fast forward #155

Merged
merged 4 commits into from
Apr 4, 2017

Conversation

yutongz
Copy link
Contributor

@yutongz yutongz commented Mar 31, 2017

@istio-testing
Copy link
Collaborator

Jenkins job istio-testing/pr-master passed

func (h helper) createBranchForFastForward(commit *string) (*string, error) {
count := 0

for true {
Copy link
Contributor

@sebastienvas sebastienvas Apr 1, 2017

Choose a reason for hiding this comment

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

please use something like fastForward/SHORTSHA

@istio-testing
Copy link
Collaborator

This is a test.

@istio-testing
Copy link
Collaborator

Jenkins job istio-testing/pr-master passed

Copy link
Contributor

@sebastienvas sebastienvas left a comment

Choose a reason for hiding this comment

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

PTAL

@istio-testing
Copy link
Collaborator

This is a test.

@istio-testing
Copy link
Collaborator

Jenkins job istio-testing/pr-master passed

@@ -93,16 +93,42 @@ func newHelper(r *string) (*helper, error) {
}
}

// Create a new branch for fast forward
func (h helper) createBranchForFastForward(commit *string) error {
branchName := fmt.Sprintf("fastForward-%s", (*commit)[0:7])
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do something like fastForward/${headBranch ie master}-GIT_SHORT_SHA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out we can't use "/" in branch name

@@ -93,16 +93,42 @@ func newHelper(r *string) (*helper, error) {
}
}

// Create a new branch for fast forward
func (h helper) createBranchForFastForward(commit *string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please pass the branch name as an argument. And generate it in createPullRequestToBase to avoid code duplication

}

newHead := fmt.Sprintf("fastForward-%s", (*commit)[0:7])
log.Printf("Created a new branch for fast forward: %s", newHead)
Copy link
Contributor

Choose a reason for hiding this comment

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

Move that logging to createBranchForFastForward

// Creates a pull request from Base branch
func (h helper) createPullRequestToBase(commit *string) error {
if commit == nil {
return errors.New("commit cannot be nil.")
}

if err := h.createBranchForFastForward(commit); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably check if the branch already exists and return nil in that case.

Copy link
Contributor

@sebastienvas sebastienvas left a comment

Choose a reason for hiding this comment

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

PTAL

@istio-testing
Copy link
Collaborator

This is a test.

@istio-testing
Copy link
Collaborator

Jenkins job istio-testing/pr-master passed

branchName := fmt.Sprintf("fastForward-%s-%s", h.Head, (*commit)[0:7])
ref := fmt.Sprintf("refs/heads/%s", branchName)

if refer, resp, err := h.Client.Git.GetRef(context.TODO(), h.Owner, h.Repo, ref); resp.StatusCode != 404 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don t think you need to actually get the ref, you can just look at the reponse from CreateRef.

Copy link
Contributor

@sebastienvas sebastienvas left a comment

Choose a reason for hiding this comment

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

Minor changes.

@istio-testing
Copy link
Collaborator

This is a test.

@istio-testing
Copy link
Collaborator

Jenkins job istio-testing/pr-master passed

@yutongz yutongz merged commit 2d6eb00 into istio:master Apr 4, 2017
@yutongz yutongz deleted the branch_fast_forward branch April 14, 2017 04:34
soloio-bot pushed a commit to soloio-bot/test-infra that referenced this pull request Jun 22, 2023
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.

None yet

3 participants