-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add keychain auth provider #3768
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
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
e202132
Add keychain auth provider
yim-lee 35ecb60
Fix keychain test condition
yim-lee 38aaf45
Use canImport instead of os check
yim-lee 5adcd36
Rename errors
yim-lee d6dd29b
Refactor url.host
yim-lee 5e049c4
Rename loadFromDisk
yim-lee e4c5262
update returns Bool
yim-lee 3070fcf
Remove addOrUpdate from protocol; append to .netrc file instead of ov…
yim-lee 7cedee1
Add CompositeAuthorizationProvider
yim-lee a43aa8c
Fix formatting
yim-lee e7249e2
Move CompositeAuthorizationProvider to SwiftTool
yim-lee a395b37
Return nil AuthorizationProvider when providers list is empty
yim-lee 993f328
Don't change current SwiftTool behavior in this PR
yim-lee f9daa1e
Rebase and adjust to ObservabilityScope changes
yim-lee 4f802bd
Fix test warnings
yim-lee 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 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
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
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
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
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
Workspace.Configuration.Netrc is designed to only concern itself with
NetrcAuthorizationProvider
, so most of the code that was removed (moved to Basics really) here should be put back, and then inSwiftTool::getAuthorizationProvider
we would create theCompositeAuthorizationProvider
from the one returned fromself.getNetrcConfig
+ the keychain one, i.e.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 undid the change in
load
, butNetrcAuthorizationProvider
can be generic?Quoting SE-0292:
So potentially in
SwiftTool::getAuthorizationProvider
we'd need to add anotherNetrcAuthorizationProvider
that points to the user-level netrc.Uh oh!
There was an error while loading. Please reload this page.
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.
Amended 6cc7ece
Uh oh!
There was an error while loading. Please reload this page.
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.
making
NetrcAuthorizationProvider
generic makes senseI think the local vs. user netrc file logic could go into
which now loads the netrc by either a flag passed to the CLI or the user level, but this can be extended to also include the local case ?
Uh oh!
There was an error while loading. Please reload this page.
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 actually think that with this change
Workspace.Configuration.Netrc
becomes redundant? any reason why not simply remove it?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.
My original intent with this PR is to add keychain auth capability. I didn't plan to change existing behavior or cover new ones.
Uh oh!
There was an error while loading. Please reload this page.
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.
Workspace.Configuration.Netrc
might be used elsewhere through libSwiftPM? It doesn't feel right to me that onlySwiftTool
needs this logic.That code is for
SwiftTool
only and I assumeSwiftPackageRegistryTool
will have to create its ownAuthorizationProvider
in a similar way. I don't know if the logic for both tools is supposed to be the same, but my current understanding is that they are not.AFAICT
options.netrcFilePath
isn't necessarily a path within the workspace, soSwiftTool
uses either a custom netrc file location, which can be anywhere, or tries to find one in user's home directory. Regardless, it uses only one netrc file.SwiftPackageRegistryTool
IIUC, looks for netrc file in both the workspace (same as project?) and user's home directory. i.e., it might use two netrc files.If the tools were to share the same logic, then the code should be moved outside of
SwiftTool
(to where?), and IMO this kind of logic doesn't belong in the tool/"UI" level.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.
SwiftTool is a shared impl for all the CLI tools, so code there would be shared across them, but you are right that this is not needed to be dealt with in this PR - we can follow up with consolidating the logic as we make more progress