-
Notifications
You must be signed in to change notification settings - Fork 324
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
Add additional replace rules for node:* modules to custom unit node loaders. #1020
Conversation
Converted to draft to resolve whether we'd need to check for the |
Just some notes for when this is ready proper... Perhaps some of the PR message could go into the commit message? I would also prefer the Closes thing to be in the form
That makes it useful outside of GH, e.g I can click on it in an xterm and have it open in my browser. And if we ever move off of GH... See here for an example, including other tags you can use... for example you may want to add a |
3782383
to
d9a72fb
Compare
@ac000 I've updated the commit message with description from the PR, and changed the link on both the PR and the commit message to include the full URI of the github issue. This is now ready to review / merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the changes themselves look simple enough. I do think it's a good idea to give recognition to people who report issues and test fixes etc (it also gives some further points of contact down the line).
So I would add a Reported-by tag at the bottom, at which point lets move the closes there also, and then you may as well go the full hog and add your Signed-off-by also. Ultimately I'd like for us to officially adopt the DCO (not a CLA).
So the bottom of the commit message (separated by a blank line) you'd end up with
Reported-by: rommanio <https://github.com/rommanio>
Closes: <https://github.com/nginx/unit/issues/1013>
Signed-off-by: Your Name <email address>
I can't force you to do that (yet), but if you have a look through git-log(1) you'll see plenty of examples of this usage.
Hm, does GitHub understand that trailer format for automatically closing issues, or does it get confused by the full URL and angle brackets? Let me test in another repo real quick. |
GH will recognise full URL's, but doesn't recognise it being in <>'s. |
To quote https://man7.org/linux/man-pages/man7/uri.7.html
|
GH should probably learn to understand that format... lets see where to file such a request... |
I don't think GH is going to entertain a request for this. Out of curiosity, is the angle bracket format used somewhere downstream that gets picked up automatically by some tool? |
No. I used to write them without the brackets until Alex pointed me to the above and suggested we should. I can live without them if it's a problem. |
Yeah, GitHub doesn't recognize them for auto-closing. Full URLs, |
Full URLs (without the brackets) so they are not tied to and are usable outside of GH. Obviously email addresses should still be brackets and and also where we refer to GH users where we don't have an email address for them but use their GH profile link. |
Any update on this PR? If we can complete the review I will add this to 1.32 or do we need to push it further out to resolve the |
I opened a bug-report ticket regarding the |
From my point this PR is done. I'm unsure what else is needed to get this merged. |
It's just the And to avoid any ambiguity, it should look like this (for now, if and when GH fix the issue with the <>'s we can start putting them in again) [You can also loose the trailing '.' from the subject line... so] EDIT: While you're at it you might as well stick my 'Reviewed-by' on there also...
|
In that particular issue the compiled nuxt files end up importing the http module as node:http rather than http only. This bypasses unit's custom loader implementation which only check for the http or unit-http modules, and their websocket counterparts. This changeset adds replace sources for both the node:http and node:websocket import signatures. Closes: nginx#1013 Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
d9a72fb
to
f95dee7
Compare
@ac000 rebased on latest master and commit message updated |
LGTM. Minimal change, already separately reviewed. Changes requested by @ac000 have been applied. Bypassing branch protections and merging. |
Closes: #1013
In that particular issue the compiled nuxt files end up importing the http module as
node:http
rather thanhttp
only. This bypasses unit's custom loader implementation which only check for thehttp
orunit-http
modules, and their websocket counterparts.This changeset adds replace sources for both the
node:http
andnode:websocket
import signatures.