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

fix new branch ancestor handling flow #36

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

Conversation

jpaskhay
Copy link

The RETURN_CODE was not properly set in cases where the git merge-base --is-ancestor call returned successfully after a previous unsuccessful result.

I tried testing the fix by manually inserting the orb command's content directly in our CircleCI config. It seemed to work for the way I was testing, and the fix intuitively seems like it should resolve the bug. It's admittedly not quite as thoroughly tested as it could be.

Checklist

(Note none of these apply as this is a bug fix)

  • All new jobs, commands, executors, parameters have descriptions
  • Examples have been added for any significant new features
  • README has been updated, if necessary

Motivation, issues

This should fix #25 where a bug in the new branch flow is not properly finding its ancestor.

Description

Fix handling of return code such that the job iteration logic properly identifies the ancestor commit in a new branch flow.

- the RETURN_CODE was not properly set in cases where the `git merge-base --is-ancestor` call returned successfully after a previous unsuccessful result
@nikoo28
Copy link

nikoo28 commented Jul 26, 2019

Any updates when can this be merged?

@xdays
Copy link

xdays commented Aug 24, 2019

will the job fail if git merge-base --is-ancestor $COMMIT_FROM_JOB_NUM $CIRCLE_SHA1 return 1?

@jpaskhay
Copy link
Author

will the job fail if git merge-base --is-ancestor $COMMIT_FROM_JOB_NUM $CIRCLE_SHA1 return 1?

there are no error flags currently set in the embedded bash script (e.g. no set -e, etc.), and it didn't fail in my testing. if that ever changed, it certainly would fail though.

@xdays
Copy link

xdays commented Aug 29, 2019

Then I will close my PR #39 , and let's waiting for your PR to be merged. Before that, I'm afraid we have to publish our own orb.

@ArWeder
Copy link

ArWeder commented Aug 29, 2019

@xdays have you published your version of the orb ?

@xdays
Copy link

xdays commented Aug 30, 2019

@xdays have you published your version of the orb ?

yes, it's https://circleci.com/orbs/registry/orb/xdays/compare-url

@oshimayoan
Copy link

I think we have to handle return code 128 as well.

return code 128 occurs when git merge-base --is-ancestor <A> <B> running with A not registered yet. So when A coming from the job of a different branch, the current branch will not know anything about this A. I think that what makes it returning 128.

@oshimayoan
Copy link

Anyway, is this repo still maintainable? I wonder why this PR is never get reviewed by the maintainer.

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.

Still broken on first build to new branch
5 participants