FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Policy scope fix by bmeck · Pull Request #37298 · nodejs/node · GitHub

/ node Public

Policy scope fix - #37298

Closed
bmeck wants to merge 5 commits into
nodejs:masterfrom
bmeck:policy-scope-fix
ConversationCommits (5)ChecksFiles changed
Closed

Policy scope fix#37298
bmeck wants to merge 5 commits into
nodejs:masterfrom
bmeck:policy-scope-fix

Conversation

bmeck commented Feb 9, 2021

Copy link
Copy Markdown
Member

This fixes a case where repeated scope lookup for modules with multiple dependencies would fail and the manifest would look up the dependency redirector of the referrer rather than the scope. This was found in stress testing of https://github.com/bmeck/local-fs-https-imports . The reason it didn't appear to have a test before is likely that code coverage was unaffected.

bmeck added confirmed-bug Issues with confirmed bugs. policy Issues and PRs related to the policy subsystem. labels Feb 9, 2021
vdeturckheim approved these changes Feb 9, 2021
View reviewed changes

vdeturckheim left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

bmeck added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 10, 2021

nodejs-github-bot commented Feb 11, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36061/

nodejs-github-bot commented Feb 12, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36063/

bmeck commented Feb 12, 2021

Copy link
Copy Markdown
Member Author

CI failure seems to be flaking and not related to this PR. Will merge in a few unless otherwise commented

bmeck commented Feb 12, 2021

Copy link
Copy Markdown
Member Author

ah, nm needs 1 more collab approval, missed that (will go poking around for one)

jasnell reviewed Feb 14, 2021
View reviewed changes
Comment thread lib/internal/policy/manifest.js Outdated
jasnell reviewed Feb 14, 2021
View reviewed changes
Comment thread lib/internal/policy/manifest.js Outdated
jasnell approved these changes Feb 14, 2021
View reviewed changes

jasnell left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Couple of minor nits but LGTM

bmeck and others added 3 commits February 15, 2021 09:10
Update lib/internal/policy/manifest.js
925b2c8 
Co-authored-by: James M Snell <jasnell@gmail.com>
Update lib/internal/policy/manifest.js
bfb4aa5 
Co-authored-by: James M Snell <jasnell@gmail.com>
fixup on nits
ac2a32b

nodejs-github-bot commented Feb 15, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36144/

bmeck commented Feb 15, 2021

Copy link
Copy Markdown
Member Author

CI issues seem unrelated, going to merge in a bit

bmeck added a commit that referenced this pull request Feb 15, 2021
policy: fix cascade getting scope
f24e707 
PR-URL: #37298
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>

bmeck commented Feb 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Landed in fc574dc58380

bmeck closed this Feb 15, 2021

Trott commented Feb 16, 2021

Copy link
Copy Markdown
Member

Landed in fc574dc58380

Landed in f24e707

danielleadams pushed a commit that referenced this pull request Feb 16, 2021
policy: fix cascade getting scope
799b2d5 
PR-URL: #37298
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>
This was referenced Feb 16, 2021
Closed
Merged
targos pushed a commit that referenced this pull request May 1, 2021
policy: fix cascade getting scope
506f1d4 
PR-URL: #37298
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>
danielleadams mentioned this pull request May 3, 2021
Merged
bmeck deleted the policy-scope-fix branch February 3, 2022 21:46
wedataintelligence mentioned this pull request Jan 12, 2023
Open
This was referenced Jan 12, 2023
Open
Open
137717unity mentioned this pull request Jan 13, 2023
Open
This was referenced Jan 13, 2023
Open
Open
Open
Open
Open
This was referenced Apr 18, 2023
Open
Open
137717unity mentioned this pull request Apr 18, 2023
Open
wedataintelligence mentioned this pull request Apr 18, 2023
Open
This was referenced Apr 19, 2023
Open
Open
ron-a20 mentioned this pull request Apr 19, 2023
Open
This was referenced Apr 19, 2023
Open
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
marsonya marsonya approved these changes
+1 more reviewer
vdeturckheim vdeturckheim approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. confirmed-bug Issues with confirmed bugs. policy Issues and PRs related to the policy subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL