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

build: do not lint src dir for JS errors #8128

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 16, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

Description of change

There are no JavaScript files in the src directory. It can be
safely omitted from the JavaScript linting step.

There are no JavaScript files in the `src` directory. It can be
safely omitted from the JavaScript linting step.
@Trott Trott added the build Issues and PRs related to build files or the CI. label Aug 16, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Aug 16, 2016

LGTM

@jbergstroem
Copy link
Member

LGTM if ci passes.

@silverwind
Copy link
Contributor

LGTM

@Trott
Copy link
Member Author

Trott commented Aug 16, 2016

@jbergstroem
Copy link
Member

@Trott bro join the game – it says so below! "11 successful checks"

@Trott
Copy link
Member Author

Trott commented Aug 17, 2016

@jbergstroem I know but I have a few reservations with relying on that:

  • Race condition: If someone else starts another CI for this PR (either by accident by mistyping their PR_ID in the Jenkins form, or by starting it at about the same time I start mine so we have two running nearly in parallel), it will (or might) overwrite the results. I don't want the successful CI run to be lost.
  • It's not clear to me that the GitHub display is permanent. When the CI results are wiped cleared in a few weeks, will it still show here as green? I imagine it will, but I don't actually know that. And since I've definitely come across PRs in the past that land and I'm all like, "Hey, this didn't get a CI run" (which I usually notice because the PR broke the CI)... you know, I want to be able to look at this one in a year and know that it had a green CI run.

These concerns may be me inventing things to worry about and in another three days, I'll be all like, "Hey, why are you posting the CI results in a comment? They're right there below. Johan worked hard on that. Don't waste our time with email notifications telling us what's plainly there in the comment. Who does that?!?!" But I'm not there yet.

I used "all like" twice in this comment. Deal with it.

@addaleax
Copy link
Member

Also, if I observed it correctly, updating PRs – be it by adding another commit or by rebasing it – does clear the status display on Github, too. I think that alone is reason enough to keep posting these comments.

@jasnell
Copy link
Member

jasnell commented Aug 17, 2016

LGTM

@jbergstroem
Copy link
Member

@Trott: I actually didn't know that the build checks were removed in time. Is that really the case? What I do know is that once we set another pending job with the same identifier the old job success/fail will be removed.

Regarding parallel jobs to the same pr: I guess we can add some barrier in the gh bot, but that check probably should live at Jenkins.

@Trott
Copy link
Member Author

Trott commented Aug 17, 2016

@jbergstroem I guess the bottom line is that how the status gets posted to the PR is currently largely a mystery to me so I'm being cautious.

Maybe I should get more involved on that end of things...

@jbergstroem
Copy link
Member

@Trott we'll figure it out as we go. @addaleax's point about keeping build info around for older CI runs (if we rebase, etc) is relevant though. Not sure how to handle that 'automated'.

Trott added a commit to Trott/io.js that referenced this pull request Aug 18, 2016
There are no JavaScript files in the `src` directory. It can be
safely omitted from the JavaScript linting step.

PR-URL: nodejs#8128
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Aug 18, 2016

Landed in dbbbf21

@Trott Trott closed this Aug 18, 2016
evanlucas pushed a commit that referenced this pull request Aug 24, 2016
There are no JavaScript files in the `src` directory. It can be
safely omitted from the JavaScript linting step.

PR-URL: #8128
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

labeling don't land, please feel free to batch in with other linting updates if desired

@Trott Trott deleted the nojsinsrc branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants