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

Update yarn.lock #432

Merged
merged 1 commit into from
Dec 9, 2019
Merged

Update yarn.lock #432

merged 1 commit into from
Dec 9, 2019

Conversation

samselikoff
Copy link
Contributor

No description provided.

@RobbieTheWagner RobbieTheWagner merged commit 8fa6fdc into master Dec 9, 2019
@RobbieTheWagner RobbieTheWagner deleted the update-yarn-lockfile branch December 9, 2019 16:39
@samselikoff
Copy link
Contributor Author

@rwwagner90 hey – did the tests pass? I see an x here and it links to https://travis-ci.org/ember-learn/ember-cli-addon-docs/builds/622701352?utm_source=github_status&utm_medium=notification

@samselikoff
Copy link
Contributor Author

We just had an issue with master broken and there was a PR merged with a failing test, we should not be merging PRs if the tests don't pass...

@samselikoff
Copy link
Contributor Author

It looks like yarn was down when it ran, and the push build broke (the PR build passed). Next time instead of merging let's restart the push build and make sure it passes as well before merging!

@RobbieTheWagner
Copy link
Member

RobbieTheWagner commented Dec 9, 2019

@samselikoff two builds ran, one for PR and one for push. I observed that one passed and one failed. I then inspected them to see what happened and noticed it was totally all passing. Don't worry, I do not merge things that are failing 😃

@samselikoff
Copy link
Contributor Author

What do you mean you noticed it was all passing? Here's the failing push build, the failing job should have been retried and the rest of the jobs ran.

The reason I'm pressing is because master was broken recently which was causing issues for some folks. If there's a failed job (either PR build or Push build) we should retry or investigate. Usually failures mean something, we don't want to end up with a flaky CI on this project.

@RobbieTheWagner
Copy link
Member

@samselikoff I mean two builds were run, and we clearly do not need both builds to pass since they are duplicates of one another. 1008, the one you referenced, did have a hangup and fail, but 1009 passed.

I know master was broken, but the PR I merged in that the failures started after was all green. I do not merge PRs with a broken build. We are on the same page on definitely not merging PRs that are broken.

On a somewhat related note, we should probably remove one of the builds, so we do not get two travis builds per PR.

@samselikoff
Copy link
Contributor Author

They are not duplicates, one runs the pushed branch of the code, the other runs the code as if it had just been automerged with master. Different things. Travis does let us disable one of them (if we kept only one it should be the PR build) but I've never done that. We should check what other infra teams in Ember ecosystem do here.

In any case, as long as there are two builds we should have just clicked replay on the failed build, instead of merging it in, because they're not always the same. The two builds are running the tests on the code in two distinct states.

@RobbieTheWagner
Copy link
Member

one runs the pushed branch of the code, the other runs the code as if it had just been automerged with master

I was unaware that they were different. I thought it was just a Travis config issue, and if you did not disable one or the other, you ended up with two builds. I've been disabling one for years, so PR builds did not take double the time.

I believe these lines from the current default Ember blueprint disable one of the builds: https://github.com/ember-cli/ember-cli/blob/master/blueprints/addon/files/.travis.yml#L25-L29

@samselikoff
Copy link
Contributor Author

I don't think those lines control it, however looking at their recent PR merges it does look like they only have PR builds enabled.

Went ahead and disabled push builds in the UI 👍

image

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.

2 participants