Skip to content

doc: improved flow regarding firewall script for macOS in building readme - #18689

Closed
JMGordon wants to merge 3 commits into
nodejs:masterfrom
JMGordon:build-doc-update
Closed

doc: improved flow regarding firewall script for macOS in building readme#18689
JMGordon wants to merge 3 commits into
nodejs:masterfrom
JMGordon:build-doc-update

Conversation

@JMGordon

@JMGordonJMGordon commented Feb 10, 2018

Copy link
Copy Markdown

Moved some of the text around regarding setting up firewall rules on macOS to help
improve text flow.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-botnodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Feb 10, 2018
@daynin

Copy link
Copy Markdown
Contributor

@JMGordon hi! I think you should check these two options too:

  • documentation is changed or added
  • commit message follows commit guidelines

Comment threadBUILDING.md Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit suggestion:

After building, setting up [filewall rules](tools/macosx-firewall.sh) can avoid popups
asking to accept incoming network connections when running tests.

@jasnell

Copy link
Copy Markdown
Member

Looks like tools/remark-cli/package-lock.json was accidentally added to the commit?

Comment threadBUILDING.md Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm assuming this was a typo in the suggestion from @jasnell and that this should have remained as firewall rules.

@mmarchini

Copy link
Copy Markdown
Contributor

@JMGordon would you be so kind and do a rebase to resolve the conflict in BUILDING.md?

@trivikr

Copy link
Copy Markdown
Member

ping @JMGordon

@BridgeAR

Copy link
Copy Markdown
Member

I went ahead and rebased and added a commit that addressed the typo plus a suggested change from myself. So @jasnell PTAL

@BridgeAR

Copy link
Copy Markdown
Member

@jasnell

Copy link
Copy Markdown
Member

still lgtm

trivikr pushed a commit that referenced this pull request Apr 15, 2018
PR-URL: #18689
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@trivikr

Copy link
Copy Markdown
Member

Landed in b003c86

Congratulations @JMGordon for your first commit in Node.js core! 🎉🎉🎉

@trivikrtrivikr closed this Apr 15, 2018
jasnell pushed a commit that referenced this pull request Apr 16, 2018
PR-URL: #18689
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
PR-URL: nodejs#18689
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.docIssues and PRs related to the documentations.toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@JMGordon@daynin@jasnell@mmarchini@trivikr@BridgeAR@richardlau@nodejs-github-bot