-
Notifications
You must be signed in to change notification settings - Fork 557
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
add workaround refs changed files #599
Conversation
ping @Ana06 |
# this Action may throw the below error, e.g. when not properly rebased | ||
# however, it still gets the modified files and we can continue | ||
# Error: The head commit for this pull_request_target event is not ahead of the base commit. | ||
continue-on-error: true |
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 think we should approach this differently, as this could hide other relevant errors. This check can be easily removed in the action (there is even a PR upstream, as jitterbit/get-changed-files@v1
behaves the same: jitterbit/get-changed-files#25). I personally try to rebase branches often (to avoid errors because not testing my changes with the latest code) and find this failure a nice reminder that I have to rebase. But as you said, it is not needed for the action to work. So, I can chery-pick the change in jitterbit/get-changed-files#25 and release Ana06/[email protected]
.
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.
great if they fix it upstream, the arguments in the linked issue make sense. if the fix is ready soon I'm fine with that otherwise we can use this workaround to ease our development
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 don't know if it will make it upstream. The reason why I released my own version is because the project didn't seem to be maintained. I have released Ana06/[email protected] which includes the fix. We are already using a fork of upstream, so let use my new version and change it to upstream if they release a version which include the same fixes.
Use Ana06/[email protected] which removes the _head commit is ahead of the base commit_ check. This made the action failed in not up-to-date branches (in which rebasing is needed). It supersedes mandiant#599
similar to the workaround in capa-rules
for instance observed in #598
Checklist