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.

Sign up for GitHub

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

Jump to bottom

Add core-validate-commit to node-test-pull-request #793

Closed
mhdawson opened this issue Jul 12, 2017 · 10 comments
Closed

Add core-validate-commit to node-test-pull-request #793

mhdawson opened this issue Jul 12, 2017 · 10 comments
Labels
ci-job-request ci-public

Comments

@mhdawson
Copy link
Member

Was thinking we should probably add core-validate-commit to node-test-pull-request to automate checks on the commit message.
mhdawson referenced this issue in nodejs/node Jul 12, 2017
@mhdawson
n-api: Implement stricter wrapping
d5b397c 
Use a stronger criterion to identify objects in the prototype chain that store
pointers to native data that were added by previous calls to `napi_wrap()`.

Whereas the old criterion for identifying `napi_wrap()`-injected prototype
chain objects was to consider an object with an internal field
count of 1 to be such an object, the new criterion is to consider an object
with an internal field count of 2 such that the second field holds a
`v8::External` which itself contains a pointer to a global static string unique
to N-API to be a `napi_wrap()`-injected prototype chain object.

This greatly reduces the possibility of returning a pointer that was not
previously added with `napi_wrap()`, and it allows us to recognize that an
object has already undergone `napi_wrap()` and we can thus prevent a chain of
wrappers only the first of which is accessible from appearing in the prototype
chain, as would be the result of multiple calls to `napi_wrap()` using the same
object.

PR-URL: #13872
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
@joaocgreis
Copy link
Member

+1

I remember some discussion around this, but I can't find it. @evanlucas do you remember? If it was simply not ready at the time, is it now?

@gibfahn
Copy link
Member

gibfahn commented Jul 13, 2017

The job is there, it just needs to be included (as long as it's working). I see that @evanlucas has access to it.

https://ci.nodejs.org/job/node-test-commitmsg/

@gibfahn
Copy link
Member

gibfahn commented Jul 13, 2017

@Trott thanks for the suggestion. Have installed it now. Do you use it when reviewing as well or just as a sanity check for the final land ?

I also wonder if we could add running this to the node-test-pull-request in addition to the linter ?

@mhdawson in reply to nodejs/node@d5b397c#commitcomment-23061905

I use it as a sanity check before landing. It'll fail before then anyway as it won't find the PR-URL or Reviewed-By lines. I think @refack has it set up as a pre-commit hook (so you can't commit without it being correct), though I could be misremembering.

@mcollina
Copy link
Member

I am -1 with this change as proposed. This will fail for all the PRs on the first go, because they lack reviews and a PR url. It will just increase the amount of nit-picking, as we will likely require to fix those details by the author, while we currently fix them on landing.

If we can disable those checks for the job, then I am +1.

@refack
Copy link
Contributor

refack commented Jul 13, 2017 •
edited
Loading

I think @refack has it set up as a pre-commit

pre-push, and it has a if ($2.includes('/nodejs/node')).
I don't want to tie my hands completely.

P.S. if it could be added as a warning to the linter job that might be better. Or if at some point we make a node-land job (could take a forced-pushed PR or an arbitrary branch in the landers repo) that's the more appropriate place.

@evanlucas
Copy link

It'll fail before then anyway as it won't find the PR-URL or Reviewed-By lines.

You can pass the --no-validate-metadata flag and it won't fail from a missing PR-URL or missing Reviewed-By.

The job that is there works pretty well. It can be run against a single commit or all of the commits for a PR. It becomes tricky with fixup commits though...

@fhinkel
Copy link
Member

fhinkel commented Jul 14, 2017

core-validate-commit is great! But iIrc it complains about Chromium backports because a line with the URL is too long.

@evanlucas
Copy link

@fhinkel I’ll try to get around to fixing that.

@maclover7 maclover7 mentioned this issue Jun 9, 2018
Closed
@richardlau richardlau mentioned this issue Oct 29, 2019
Merged
@sam-github
Copy link
Contributor

Closing as stale, but if anyone wants to take this up feel free to reopen.

@richardlau
Copy link
Member

We are running core-validate-commit in Travis CI but it was recently marked as allowed to fail which means that you need to manually look at the Travis CI results (either the check tab on the PR page or in Travis itself) to see if it failed or not (the overall check status for Travis CI in the main tab on the PR page will be green even if the core-validate-commit run fails).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
ci-job-request ci-public
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@sam-github @mcollina @refack @fhinkel @joaocgreis @evanlucas @maclover7 @richardlau @mhdawson @gibfahn