Skip to content

Fix broken build #10541

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

Merged
merged 1 commit into from
May 25, 2019
Merged

Fix broken build #10541

merged 1 commit into from
May 25, 2019

Conversation

BrennanConroy
Copy link
Member

Guess we shouldn't be merging 3 month old PRs without rebuilding 😆
#7227

@Pilchie
Copy link
Member

Pilchie commented May 25, 2019

Thanks for being on top of it @BrennanConroy

@BrennanConroy BrennanConroy merged commit 833ddbe into master May 25, 2019
@BrennanConroy BrennanConroy deleted the brecon/fixSample branch May 25, 2019 21:47
@jkotalik
Copy link
Contributor

Guess we shouldn't be merging 3 month old PRs without rebuilding 😆

This is a legitimate problem though, even with shorter time gaps. There are many times where we have merged a PR shortly after another PR has been merged and it causes the build to break, even though the PR is green.

A few general ideas on how we can make this experience better:

  • Have a bot that can detect when there is broken build and immediately merge a revert commit.
  • Have a PR check that is retriggered every time something is committed to master. It only does builds though and not tests.
  • Have a check that says a branch is stale and blocks merging the PR until the check has been rerun.

@analogrelay
Copy link
Contributor

Ah crap, my bad. I respun a different PR and thought that I had done so with this one.

@analogrelay
Copy link
Contributor

I also feel like we should try to do a better job of closing/handling stale PRs. For the most part, after a month or so without merging a (non-draft) PR, it should probably just be closed. If the change is still desired, it's easy to either re-open or open a new PR.

@Pilchie
Copy link
Member

Pilchie commented May 26, 2019

Have a check that says a branch is stale and blocks merging the PR until the check has been rerun.

GitHub has a built-in option for this, but in practice, it ends up being very frustrating to enable, unless PR checks complete in a very short time.

@analogrelay
Copy link
Contributor

analogrelay commented May 26, 2019

I think what GitHub has is a feature to invalidate PR checks whenever something new is pushed to the target branch. I agree that is too intense here. Something that says "you can't merge a week-old/month-old/etc PR unless you respin all the checks" might be reasonable if it could be done well.

Honestly, I don't know that this is that much of a problem as long as those doing the merging (who merged this PR in the first place anyway! oh... wait...) do the due diligence. I don't think this has been a common problem and I certainly feel adequately shamed into doing the right thing next time 😺 .

@Pilchie
Copy link
Member

Pilchie commented May 27, 2019

At one point @dpoeschl had a Chrome Ext that warned when the checks were stale, but agree that just a bit of diligence is probably enough

@Tratcher
Copy link
Member

I've hit this several times with compiler breaks and behavior breaks. Preventing both kinds requires very invasive mechanics and would make normal development almost imposible. As the fixes have always been trivial, quick notification and reaction should be sufficient. Can the official build automatically email the committer on failure?

@BrennanConroy
Copy link
Member Author

It should already do that. At least I get them when I merge a PR and it fails.

@analogrelay
Copy link
Contributor

Yep, it's all configurable in AzDO settings. The default should be emailing you on failure though.

@natemcmaster
Copy link
Contributor

We have ways of working around GitHub limitations. We could, for example, use the aspnet-github-bot account to add a required PR check that asserts the PR check passed within the last 7 days.

@jkotalik
Copy link
Contributor

We could, for example, use the aspnet-github-bot account to add a required PR check that asserts the PR check passed within the last 7 days.

That was exactly my thought 👍

@analogrelay
Copy link
Contributor

Yep, that would be the way to move forward if we felt like it added enough value to prioritize it over other stuff. I'm just not sure it's the main issue we're facing right now ;P.

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.

6 participants