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

New Linter strategy #2001

Closed
rvagg opened this issue Oct 25, 2019 · 13 comments
Closed

New Linter strategy #2001

rvagg opened this issue Oct 25, 2019 · 13 comments
Labels

Comments

@rvagg
Copy link
Member

rvagg commented Oct 25, 2019

The linter jobs have been stuck on FreeBSD 10 since we first introduced them. We need to get rid of FreeBSD 10, it's way out of support and we don't have anyone who is interested in maintaining them anyway. There was an aborted attempt to move them to Ubuntu machines but there were specifics about the various make lint commands that caused problems with that (we got ourselves into a BSD corner). It seems that we're past that and the linter(s) are all nicely BSD+GNU. So it's time to move them or have a new strategy.

There are two possible paths I want to put forward:

  1. Run the lint jobs on Ubuntu machines (the jenkins-workspace machines might be a good choice). This would mean (1) making sure there is a node on those machines, and (2) reconfiguring the job to use the new machines.
  2. Run the liters in Docker using some of the infra that comes in Proposal for major changes to (and expansion of) containerised build system #1982 - namely the ability to execute a particular container that gets pulled from Docker Hub. I've set up a linter Dockerfile here: https://github.com/rvagg/node-ci-containers/tree/master/node-linter (in Docker Hub as rvagg/node-ci-containers:node-linter, again, that 'rvagg' will change). You just run it with your directory mounted: docker run -v $(pwd):/home/iojs/workspace rvagg/node-ci-containers:node-linter. This gets our linting logic out of Jenkins and into a form that's accessible and usable by collaborators and gives us an easier upgrade path in the future.

When this is done, I want to purge FreeBSD 10 from our infra. There's some urgency to this too btw since we need to move some Joyent resources around in the next few weeks and there's a FreeBSD 10 / linter machine involved in that.

@jbergstroem
Copy link
Member

FWIW: there's nothing about the linters that is tied to FreeBSD; it was just a personal preference when I originally set it up.

Run the liters in Docker using some of the infra that comes in #1982 - namely the ability to execute a particular container that gets pulled from Docker Hub.

I like this approach. It makes it simpler to drop in any shared infra from other CI's or github actions.

@sam-github
Copy link
Contributor

caveat: I may be missing something about the history and requirements, for example, I'm honestly not sure if git node land requires a green Travis run. I don't think I've ever seen Travis red and Jenkins green, so haven't run into that :-).

Why are we setting this up in jenkins? Can linting be left to be done only on Travis?

Travis only is simpler (delete the jobs, Travis does make lint-py doc-only lint already), and for the lint jobs in particular, they only need one platform and Travis already gives immediate feedback on a PR without collaborator intervention.

@richardlau
Copy link
Member

caveat: I may be missing something about the history and requirements, for example, I'm honestly not sure if git node land requires a green Travis run. I don't think I've ever seen Travis red and Jenkins green, so haven't run into that :-).

node-core-utils currently doesn't check Travis results. We don't require Travis to pass to land a PR but there is a proposal (nodejs/node#29770) to allow just Travis for doc-only changes.

@richardlau
Copy link
Member

Would https://github.com/rvagg/node-ci-containers/tree/master/node-linter be simpler if it used a Node.js docker container?

@sam-github
Copy link
Contributor

sam-github commented Oct 25, 2019

Wouldn't node-core-utils be simpler if it only tested (E"DIT: "checked") GH build results? I'm sure there is a reason its reaching out to Jenkins, I'm just not sure what it is.

@richardlau
Copy link
Member

Wouldn't node-core-utils be simpler if it only tested (E"DIT: "checked") GH build results? I'm sure there is a reason its reaching out to Jenkins, I'm just not sure what it is.

Yes, but it currently does not even look at GH checks. nodejs/node-core-utils#369 would have to land first.

@rvagg
Copy link
Member Author

rvagg commented Oct 26, 2019

@richardlau

Would https://github.com/rvagg/node-ci-containers/tree/master/node-linter be simpler if it used a Node.js docker container?

Yes it would, I think, I don't know why I didn't think of that.

@sam-github wrt Travis, some history: Travis support came much later than Jenkins because we had a policy of no-app-access for the org and that excluded Travis. That policy changed when GitHub gave more fine-grained control over access for orgs (or something like that), so we inherited the .travis.yml from ayo.js at some point between late 2017 and late 2018. I never followed that and it was pretty much entirely out of the perview of Build and has mostly remained as such. That fact alone has driven the disconnect and lands us in this issue! I hadn't even thought of Travis as an option here.

Travis is nice for the fact that we don't have to worry at all about the security concerns. I really don't like having the two overlapping services side-by-side and not integrated at all (a beefed-up GitHub Actions with full support for custom agents on arbitrary servers we manage so we can retire Jenkins is the dream).

SO, with all that, I guess that option (3) is to remove the linter job from Jenkins entirely and rely on Travis. That would certainly aid in the reduction of the complexity of Build's responsibility. It even makes sense that a quick lint job lives in Travis and gives immediate feedback to pull requests than having to go through our CI. A variation of this option is to have it in Travis and put it in .ci.yml (#1982) for the sake of completeness in Jenkins and not have a dedicated job for linting. Quick running jobs in .ci.yml are an ideal use-case and I'm assuming collaborators will want to add more things in there like this.

@rvagg
Copy link
Member Author

rvagg commented Oct 26, 2019

FWIW: there's nothing about the linters that is tied to FreeBSD; it was just a personal preference when I originally set it up.

OK, I remember now that our hassle was in the Jenkins setup being BSD-specific. We tried to introduce Ubuntu 16.04 machines to the mix to migrate but discovered how hard it was to have both BSD and GNU tooling in the same scripting. I'm pretty sure Refael ended up sorting all of that out, the scripting in there now looks for every tool and adapts as needed. We just never committed to the migration.

Also folks remember that there are different linting tasks for different release lines, so it's not quite as simple as what's in Travis. lint-py-build is only in newer release lines for instance. I think this release-line specificity argues for Travis and/or .ci.yml so those particulars can be baked in to the release line and not in Jenkins or some script we put in this repo.

@rvagg
Copy link
Member Author

rvagg commented Oct 26, 2019

OK, so it turns out that from 8.x onward we have a solid enough Makefile and lint-ci that we don't need much of the additional fluff currently in the lint script. I've boiled it down to just this which is really just make lint-ci and some output to stdout if it fails.

Here's it failing on 8.x with a JS lint error: https://ci.nodejs.org/job/rv-test-commit-linux-docker/78/label=docker-host-x64,linux_x64_container_suite=lint/console
And passing: https://ci.nodejs.org/job/rv-test-commit-linux-docker/79/label=docker-host-x64,linux_x64_container_suite=lint/console

The script simplification makes me a bit more comfortable having an interim measure of moving the linting onto the jenkins-workspace machines, they'll need node installed, however. Then we can get rid of FreeBSD 10. We can then decide whether we want to migrate lint to .ci.yml or just Travis.

@rvagg
Copy link
Member Author

rvagg commented Oct 27, 2019

#2008

prepared the 3 jenkins-workspace machines, have switched label in the Jenkins job, simplified the script a bit, and they've taken over linting now.

In the process I also realised my previous comment about simplification isn't quite right, lint-py-build needs to be run separately and both it and lint-py need to be run with python3 on Node >= 12 according to the script in the Jenkins job. So I'm putting that into my .ci.yml proposal too.

@github-actions
Copy link

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

@github-actions github-actions bot added the stale label Aug 23, 2020
@mmarchini
Copy link
Contributor

node-core-utils will check Actions after the next release, so in theory we can remove linter jobs from Jenkins as those are already covered by Actions

@mmarchini mmarchini removed the stale label Aug 23, 2020
@github-actions
Copy link

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants