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

Logstash versioned plugins build failures vs auto-PR and required manual cleanup #21

Open
Tracked by #48
yaauie opened this issue Jul 17, 2018 · 1 comment
Open
Tracked by #48
Assignees

Comments

@yaauie
Copy link
Member

yaauie commented Jul 17, 2018

With a constant branch name introduced in 114fe57, once the branch exists on origin, subsequent failed builds will fail to push their changes to the constant branch, since the push is not a fast-forward of the existing dereference.

This prevents the diff for the existing PR from being updated, meaning that once a build fails attempts to fix the build are outside the PR feedback loop.

Additionally, after a failed-build-PR is merged or closed, users must remember to manually clean up the branch (e.g., by deleting it in the Github UI or by pushing a deletion with git push origin :versioned_docs_failed_build), or else subsequent build failures will open up a new PR that points to a previous failure.

Should we add a --force to the git push command when opening that PR? This would cause the in-flight PR to get updated on each subsequent build failure, but the nature of a force-push could be surprising to anyone attempting to manually fix-up the PR.

@jsvd
Copy link
Member

jsvd commented Jul 30, 2018

This prevents the diff for the existing PR from being updated, meaning that once a build fails attempts to fix the build are outside the PR feedback loop.

I introduced this change for a single branch because the jenkins job was being run every 6 hours and, if it started failing, it would create a new (but identical) broken branch and pr. The way it is now, the broken build will create the PR and you can solve the broken-ness by editing the PR directly and merging it.

Additionally, after a failed-build-PR is merged or closed, users must remember to manually clean up the branch (e.g., by deleting it in the Github UI or by pushing a deletion with git push origin :versioned_docs_failed_build), or else subsequent build failures will open up a new PR that points to a previous failure.

I agree..this is annoying, obscure and undocumented..maybe we can improve this by editing the PR message with both the result of the perl script build failure message and a guideline on fixing it (including a reference to deleting the branch)?

Should we add a --force to the git push command when opening that PR? This would cause the in-flight PR to get updated on each subsequent build failure, but the nature of a force-push could be surprising to anyone attempting to manually fix-up the PR.

My thoughts on not doing a --force is that this way whoever debugs the failure will take care of the first error preventing the build from completing. If you fix it, the next build run will either succeed or fail on a separate problem. Rinse, repeat.
This 1 error per build is useful because that's also how the perl scripts work, if there is 1 error, it reports it and aborts (even if there are 10 errors).

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

No branches or pull requests

2 participants