resume-schema icon indicating copy to clipboard operation
resume-schema copied to clipboard

failing test script if there is lint in the code or in the schema

Open antialias opened this issue 4 years ago • 8 comments

I recommend merging https://github.com/jsonresume/resume-schema/pull/378 first

antialias avatar Apr 24 '20 20:04 antialias

We should avoid OS-specific tooling (ex shell scripts, CLI utils). CI is pretty barebones as-is and may be changed to include Windows as well.

evanplaice avatar Apr 24 '20 21:04 evanplaice

We should avoid OS-specific tooling (ex shell scripts, CLI utils)

Why? Banning shell scripts implies not using scripts in package.json at all - they are all run as shell scripts by npm.

Also, ubuntu-latest is declared as as the OS which runs the checks, so there's no reason to try to make all the OSes happy at CI time.

CI ... may be changed to include Windows as well.

Then let's decide now not use Windows to run our lint checks but since Windows runs bash natively now, they would probably work as is anyway.

antialias avatar Apr 24 '20 22:04 antialias

Why? Banning shell scripts implies not using scripts in package.json at all - they are all run as shell scripts by npm.

package.json scripts that execute Node scripts are OS independent

Then let's decide now not use Windows to run our lint checks but since Windows runs bash natively now, they would probably work as is anyway.

Windows Subsystem for Linux can run bash scripts. That's not the default CLI shell in Windows; it's Windows 10 only, and it requires extra setup. If you'd like, I can add windows to the OS matrix in the GH Actions config to demonstrate.


Why? Including unix-specific scripting blocks Windows-based users and contributors from being able to run the tests.

evanplaice avatar Apr 24 '20 22:04 evanplaice

package.json scripts that execute Node scripts are OS independent

Can you be more specific about what you want / don't want here? The executables referenced in scripts are declared in package.json so they get installed by npm install along with the rest of this project's devDependencies. Exceptions are diff and printf, which have been around in every unix-like environment for decades.

Aside from "being OS independent" (and this PR maintains that for any unix-like env), do we not want to care about PRs introducing lint into the schema? Happy to go with another solution if you can recommend something.

These scripts are only intended for CI anyways, which is well-defined as ubuntu in the github actions that you set up, so I don't understand why we need to start checking for lint on windows all of a sudden.

antialias avatar Apr 24 '20 23:04 antialias

Use Case:

A contributor (who uses Windows) is working on a PR. The PR has a mistake (ie linting error) so the CI fails as expected. How does that contributor identify what went wrong? That's why every script run on CI should also be able to run locally.

Aside from "being OS independent" (and this PR maintains that for any unix-like env), do we not want to care about PRs introducing lint into the schema?

The CI and CD checks are virtually identical. CI just breaks the preversion checks out into separate steps (ex test, lint) so it's easier to identify what failed. This is intentional, any PR that gets approved should be able to be merged in with no additional work.

Happy to go with another solution if you can recommend something.

An alternative should be JS-only. I'd probably go with a tool that uses or works like ESLint for JSON.


Personally, I haven't used a Windows for 5 years; even then it was only on work machines b/c I was required to. I would loooove if Windows just ceased to exist one day. With that said, not all devs are ready, willing, or able to ditch Windows.

evanplaice avatar Apr 24 '20 23:04 evanplaice

A contributor (who uses Windows) is working on a PR. The PR has a mistake (ie linting error) so the CI fails as expected. How does that contributor identify what went wrong? That's why every script run on CI should also be able to run locally.

Well the way I usually discover why CI failed is by looking at the output posted on the nifty github checks checks that you set up. I can view this with my test account that is not a member of the jsonresume team, so it will work for anyone.

Screen Shot 2020-04-24 at 6 53 15 PM

https://github.com/jsonresume/resume-schema/pull/377/checks?check_run_id=616829721

antialias avatar Apr 25 '20 01:04 antialias

Happy to go with another solution if you can recommend something.

I'd probably go with a tool that uses or works like ESLint for JSON

I did. Here's what's being used and why:

  • eslint-plugin-json, an eslint plugin that checks JSON for validity (sadly, it does not order the keys nor enforce pretty printing, which is why I also added)
  • jsonlint-cli sorts the json and keeps it pretty-printed.

Both of these tools are implemented in javascript and have node APIs that can be called from within a .js file if we really want to go all-js, but calling them in the right sequence and failing in the correct way is a couple orders of magnitude more complicated when done from javascript. Shell scripting has its use and one of them is writing ci checks that can be understood by looking a just a few lines of code.

antialias avatar Apr 25 '20 01:04 antialias

Why? Banning shell scripts implies not using scripts in package.json at all - they are all run as shell scripts by npm.

package.json scripts that execute Node scripts are OS independent

The scripts are executed by the shell from which npm run was invoked. For example, this package.json script won't work as expected in Windows because && has a different meaning in powershell than is does in bash:

"test": "npm run validate && npm run test-units",

(the above line is copied and pasted from package.json in the v1.0.0 branch so we're already broken in Powershell)

antialias avatar Apr 25 '20 02:04 antialias