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

Don't fail on push to master #104

Closed
matoous opened this issue Sep 11, 2019 · 5 comments
Closed

Don't fail on push to master #104

matoous opened this issue Sep 11, 2019 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@matoous
Copy link

matoous commented Sep 11, 2019

Currently the action fails on push to master branch.

Commit message:

all: Fix tests and lint

Fix failing tests and lint the whole project.

Not sure what the desired behavior here is but I would love to use this on my private repo where I push right into the master and repair failed action by ammending to the commit in case anything fails (yup.. not the best practice under the sun).

@fallion
Copy link
Member

fallion commented Sep 11, 2019

@matoous what would be the expected behaviour here? Because CI will see the last commits as equal. Check last 5 commits?

@fallion fallion changed the title Don't file on push to master Don't fail on push to master Sep 11, 2019
@fallion fallion added the bug Something isn't working label Sep 11, 2019
@fallion
Copy link
Member

fallion commented Sep 11, 2019

@matoous
Copy link
Author

matoous commented Sep 17, 2019

I see a few options here:

  1. Lint all commits to last tag.
  2. Lint only last commit.
  3. Lint X last commits.

I think that combination of 1 and 3 would be best (lint all commits until last tag but cap to 10 commits max maybe?). Either way, printing a warning should be enough to inform the user that this is not the intended use-case and that the behavior might differ from running on separate non-master branch.

@fallion fallion self-assigned this Oct 6, 2019
@aexvir
Copy link
Member

aexvir commented Nov 8, 2019

I don't think that linting all the commits to the last tag makes, even if capping it makes much sense.

I can't seem to find numbers, but I'd say that a pretty big amount of PRs usually include a single commit, so, for me, the second option makes more sense. It will end up checking the same, non-related commits many times. And it also can cause issues on projects that didn't follow the convention and just recently they added this step. Also, not a big fan of having an arbitrary number of commits checked, but that might be just me.

Linting just the last commit when there is nothing to compare against, and showing a warning would be the way to go in my opinion.

@fallion
Copy link
Member

fallion commented Nov 12, 2019

Closed by #145 and #146

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants