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

[bugfix] Fixing linting error in macro_tests #5918

Merged

Conversation

xtinec
Copy link
Contributor

@xtinec xtinec commented Sep 17, 2018

Getting master to green.
@mistercrunch @kristw

@john-bodley
Copy link
Member

@mistercrunch it seems there may be merit in having this check enabled to prevent the master branch from failing the CI checks as people may merge PRs which previously passed CI but would fail if they rebased against master.

screen shot 2018-09-17 at 6 06 14 pm

I realize that this creates some friction, i.e., PR merging becomes a sequential process and the need to rebase (and thus re-run the CI) may slow development, but it seems the safest option for ensuring that we don't have these types of regressions.

@williaster
Copy link
Contributor

+1 for @john-bodley 's comment, broken builds cost a lot of people a significant amount of time so it'd be great to add that check even if it adds a marginal per-PR cost.

@graceguo-supercat graceguo-supercat merged commit 74940e6 into apache:master Sep 18, 2018
@xtinec
Copy link
Contributor Author

xtinec commented Sep 18, 2018

Agree with @john-bodley and @williaster . I think the benefits outweigh the per-PR cost and it actually saves time for those of us in the habit of rebasing. :) Saw the lint error by chance this afternoon after I rebased in an unrelated change. :)

@mistercrunch
Copy link
Member

Wait does that mean that every single PR out there needs to be rebased every time another one is merged? If that's the case it seems like a really bad idea. I'm not even sure Travis-CI could take the extra load.

It's pretty rare that stuff like this collide. [working build + working build ~= working build] 95%+ of the time. Picture if I merge this and all other PRs have to be rebased, and we have to wait for CI on all those other builds. This is a lot more time than fixing the very occasional master.

@mistercrunch
Copy link
Member

Think about it, it means the merge button is almost never green on all PRs.

@mistercrunch
Copy link
Member

Here's another idea to mitigate this. The problem here is "a broken build", which really in most cases the broken build is really just a lint issue related to a bad merge. Maybe the solution is that we should not break the build on lint errors, and instead warn on the PR, and have a policy (either hard or softly enforced) to not merge PRs with lint.

This way, when lint shows up as a byproduct or a merge, it wouldn't break the build, it would just generate a tiny bit of lint for anyone who rebases while in this state, and those who do would fix that one-liner. That would prevent the false-negative "broken build".

@mistercrunch
Copy link
Member

@john-bodley @williaster @xtinec I'd love for you all to reconsider this thinking about the burden on all contributors and maintainers that this would involve, and how unscalable process-wise that is (the burden grows quadratically as contributions grows)

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants