-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Enable CorePack Installing Package Managers from Private Registries #11077
Merged
kbukum1
merged 26 commits into
main
from
kamil/set_corepack_registry_for_installing_npm_and_yarn
Dec 11, 2024
Merged
Changes from 1 commit
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
30d8dd2
add registry for corepack install package managers
kbukum1 07c385c
add registry helper into imports
kbukum1 8da6ed9
remove lockfile to find out registry for corepack install
kbukum1 b4031df
fix parameters in testing
kbukum1 e78b1ff
fix specs
kbukum1 05ed720
fixed spec
kbukum1 5c596bf
Merge branch 'main' into kamil/set_corepack_registry_for_installing_n…
kbukum1 234c4db
add dependabot yml credentials to check npm registry
kbukum1 9874d68
Merge branch 'main' into kamil/set_corepack_registry_for_installing_n…
kbukum1 e870538
move registry_helper into package manager helper
kbukum1 8b8b130
remove registry_helper
kbukum1 2e90495
fix reverse proxy check
kbukum1 56f8c7c
fix yarnrc issue
kbukum1 ec013e3
improve the parsing
kbukum1 def21df
moved registry helper as seperate class
kbukum1 0789da6
Merge branch 'main' into kamil/set_corepack_registry_for_installing_n…
kbukum1 7cd79a6
fix linting issue
kbukum1 16915d5
fix rubocop checks
kbukum1 6a0c2ec
Merge branch 'main' into kamil/set_corepack_registry_for_installing_n…
kbukum1 c97ef73
activate installed package manager
kbukum1 e745cde
add logging when activating the package manager
kbukum1 2878c4d
fix spec
kbukum1 02b9f2f
Merge branch 'main' into kamil/set_corepack_registry_for_installing_n…
kbukum1 1a3805e
fix logging
kbukum1 d18afbc
add feature for corepack registry
kbukum1 679cea2
add feature flag for enable registry for corepack
kbukum1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Sorry if my previous comment was confusing, I was not referring to the location of the class. What I meant to ask was why this needed to be a standalone class at all?
The class contains two instance variables which are also known by
PackageManagerHelper
, and all other logic is entirely functional in that there are no changes to stored state. The class is also instantiated in one place only - the constructor ofPackageManagerHelper
.The functional nature of the class combined with it's highly scoped usage makes me think that one of two things should be going on here:
PackageManagerHelper
then it should be instantiated outside ofPackageManagerHelper
and passed in as an argument. In this way thePackageManagerHelper
is entirely agnostic to the idea of registry credentials and configurationse.g.
PackageManagerHelper.new(package_json, lockfiles, registry_helper)
RegistryHelper
is only ever going to be used within the code ofPackageManagerHelper
, thenRegistryHelper
should not be a standalone class. If you want to isolate this logic, you could easily make the class into a module that gets included intoPackageManagerHelper
e.g.
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.
Thank you for the feedback! The reason I structured it this way is that the long-term goal is to centralize all registry-related logic within the
RegistryHelper
class. This approach allows us to handle all registry-related operations in one place, making it easier for others to extend or modify the logic in the future, should the need arise for additional registry-related functionality.By encapsulating the related constants, variables, and logic in a dedicated class, I’m aiming to keep the code clean, modular, and reusable. Using a module would require passing parameters to each function or tightly coupling the logic to
PackageManagerHelper
, which I believe would reduce flexibility and reusability.I hope this clarifies my rationale, and I’m happy to discuss further if needed!
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.
I can pass registry helper as a object into package manager helper. That can be another way of using it but I am also using similar parameters such as
lockfiles
. So that's why I initiated the registry helper in the package manager helper.