-
Notifications
You must be signed in to change notification settings - Fork 277
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
failing test script if there is lint in the code or in the schema #377
base: v1.0.0
Are you sure you want to change the base?
Conversation
3877546
to
aa240f3
Compare
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. |
aa240f3
to
d7ddf85
Compare
d7ddf85
to
ad6c6f1
Compare
Why? Banning shell scripts implies not using Also,
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. |
ad6c6f1
to
7cded48
Compare
7cded48
to
30f2e51
Compare
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. |
Can you be more specific about what you want / don't want here? The executables referenced in 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. |
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.
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.
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. |
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 https://github.com/jsonresume/resume-schema/pull/377/checks?check_run_id=616829721 |
I did. Here's what's being used and why:
Both of these tools are implemented in javascript and have node APIs that can be called from within a |
The scripts are executed by the shell from which "test": "npm run validate && npm run test-units", (the above line is copied and pasted from package.json in the |
I recommend merging #378 first