Remove merge check from circle CI configuration. #613
Conversation
jbarrick-mesosphere
requested review from
kensipe,
alenkacz,
gerred and
runyontr
There was a problem hiding this comment.
I need to think about it more but I guess it should be fine. I don't like the restriction pushed by github on you that you have to have the branch on top of master before merging. We had that enforced on Marathon once and I was just rebasing branches all the time because you merge one and all others are obsolete. So it was a lot of work and it hardly ever uncovered some bug we would caught AFTER merge. I don't remember such case...
| @@ -45,7 +37,7 @@ jobs: | |||
| - "/go/bin" | |||
| workflows: | |||
| version: 2 | |||
| formatting: | |||
| test: | |||
There was a problem hiding this comment.
should the workflow be named the same as one of the step? 🤔
There was a problem hiding this comment.
Well, I think it's an improvement over "formatting", but I'm not sold on it beyond that :)
Once we have Prow, I believe tide is able to automate the process of testing these PRs against the latest master and merging them. |
|
agreed with the tide thing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This removes the merge before running tests in Circle CI. I believe this check provides no value as it runs at commit time and not at merge time, so there are no guarantees that this even tests the correct merge target and creates difficulty with PR branches. To enforce merge safety, we should use GitHub branch settings.
Which issue(s) this PR fixes:
Fixes #568
Special notes for your reviewer:
Does this PR introduce a user-facing change?: