-
Notifications
You must be signed in to change notification settings - Fork 4
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
Handle merge commits with multiple parents and conflicts #152
Conversation
@@ -9,6 +9,7 @@ on: | |||
- '.github/workflows/sync-default.yml' | |||
- '.github/tests/*.bats' | |||
- 'private/scripts/**' | |||
- 'devops/**' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making a change to this file explicitly does not exclude this PR from running these tests because we just made a change to playwright.yml
which triggers the workflow. ¯\_(ツ)_/¯
these were likely pulled in and don't have a deletion in the history. They don't exist in the source and they don't need to exist in the upstream.
echo "File $file was deleted in the cherry-picked commit. Resolving by keeping the deletion." | ||
git rm "$file" | ||
else | ||
echo "Conflict required manual resolution for $file." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this line need to bail (or set a flag to bail after)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should bail on its own. When I was running it locally it caused the script to halt, which in Circle should cause the workflow to fail. I don't think we need a more explicit bail here.
ignored_files="composer.lock CODE_OF_CONDUCT.md CONTRIBUTING.md" | ||
|
||
# Remove ignored files from the commit. | ||
for file in $ignored_files; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be an array not a space delineated string but I care 2/10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought space separated string was how you did arrays in bash. Historically, we ran into issues creating arrays (which probably means we were __doing_it_wrong) and this is how I remember doing it so it worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this works, its just not ideal
Co-authored-by: Phil Tyler <[email protected]>
This pull request adds handling for merge commits with multiple parents and conflicts where the cherry-picked commit delets a file that does not exist in the
--theirs
version.Test deploy is here: pantheon-upstreams/wordpress-composer-managed@main...jazzsequence:wordpress-composer-managed:main
78e4862 was added because it looks like the
composer.lock
was being added in that deploy https://github.com/jazzsequence/wordpress-composer-managed