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

Run nsp check on release #4285

Merged
merged 3 commits into from
Oct 24, 2017
Merged

Run nsp check on release #4285

merged 3 commits into from
Oct 24, 2017

Conversation

flovilmart
Copy link
Contributor

As suggested by #4266

@codecov
Copy link

codecov bot commented Oct 22, 2017

Codecov Report

Merging #4285 into master will increase coverage by 10.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #4285       +/-   ##
===========================================
+ Coverage   82.41%   92.43%   +10.01%     
===========================================
  Files         118      118               
  Lines        8151     8207       +56     
===========================================
+ Hits         6718     7586      +868     
+ Misses       1433      621      -812
Impacted Files Coverage Δ
src/RestWrite.js 93.34% <0%> (ø) ⬆️
src/Controllers/UserController.js 92.98% <0%> (+0.87%) ⬆️
src/Routers/UsersRouter.js 91.66% <0%> (+1.34%) ⬆️
src/Adapters/Storage/Postgres/PostgresClient.js 85.71% <0%> (+78.57%) ⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 96.62% <0%> (+93.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6685932...db130d5. Read the comment docs.

@steven-supersolid
Copy link
Contributor

Could also look at snyk (snyk.io) although that requires sign-in.

We install snyk and nsp as dev depencies run both during build. Nsp usually catches what snyk does - not sure if they use the same vulnerabilities database.

Snyk can also watch a repository and run tests on a PR https://snyk.io/docs/github

@montymxb
Copy link
Contributor

Same as mentioned for snyk, nsp can also run on our PRs as a separate step in our CI. Believe this also requires us to signup however.

@flovilmart
Copy link
Contributor Author

This runs on PR's, that's why it fails.

@flovilmart
Copy link
Contributor Author

@steven-supersolid I merged your branch onto master and updated this one, in all goodness, this should be OK with nsp check. They also have an eslint plugin that warns on unsafe potential code.

@flovilmart
Copy link
Contributor Author

@montymxb what do you think? should we go with nsp for now, at least, next release will be safe :)

Copy link
Contributor

@acinader acinader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. very cool.

@flovilmart flovilmart merged commit 7d2332e into master Oct 24, 2017
@flovilmart flovilmart deleted the nsp-check-travis branch October 24, 2017 23:06
@montymxb
Copy link
Contributor

@flovilmart Yeah nsp works for me. I think it's good to know our options though, should we choose to look into this in further detail.

@montymxb montymxb removed their assignment Oct 24, 2017
@montymxb
Copy link
Contributor

@flovilmart at some point we'll probably want this to run on PRs too. Release checking is better than nothing, but it would be nice to know if some pending feature was about to introduce a potential issue. Was there some problem with trying it on PRs originally?

@flovilmart
Copy link
Contributor Author

This is running on PR’s, as you can see in https://travis-ci.org/parse-community/parse-server/builds/291256870

It’s only run at the release stage. This is why in this builds, we can clearly identify that we’d have issues releasing that PR, which is not related to the code itself.

@montymxb
Copy link
Contributor

Ah I stand corrected! We're all good then.

@steven-supersolid
Copy link
Contributor

Nice :)

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

Successfully merging this pull request may close these issues.

4 participants