Skip to content

Issue #34 - Support ssh git URLs and ssh key authentication#37

Merged
alexmt merged 2 commits intoargoproj:masterfrom
alexmt:34-ssh-git-urls
Mar 15, 2018
Merged

Issue #34 - Support ssh git URLs and ssh key authentication#37
alexmt merged 2 commits intoargoproj:masterfrom
alexmt:34-ssh-git-urls

Conversation

@alexmt
Copy link
Collaborator

@alexmt alexmt commented Mar 15, 2018

PR closes #34 by allowing adding repos with SSH URLs and use SSH key for authentication.

@alexmt alexmt requested a review from merenbach March 15, 2018 16:23

// IsSshURL returns true is supplied URL is SSH URL
func IsSshURL(url string) bool {
return strings.Index(url, "git@") == 0
Copy link
Contributor

@merenbach merenbach Mar 15, 2018

Choose a reason for hiding this comment

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

@alexmt The URL library for Golang has a URL parser that returns a URL struct where you can access the scheme, user, and other attributes. Wondering if this might help make this function more robust.

Edited to add: maybe Parse (which you use above) is more appropriate if we want to support URLs without a scheme attached.

Copy link
Contributor

@merenbach merenbach Mar 15, 2018

Choose a reason for hiding this comment

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

Also wondering if a unit test could be written for this function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, URL parser considers SSH URL invalid and returns error. Therefore I had to use strings.Index instead. Unit tests have been implemented.

@@ -11,20 +12,59 @@ import (

// NormalizeGitURL normalizes a git URL for lookup and storage
func NormalizeGitURL(repo string) string {
Copy link
Contributor

@merenbach merenbach Mar 15, 2018

Choose a reason for hiding this comment

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

Wondering about unit tests for this function, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@merenbach merenbach left a comment

Choose a reason for hiding this comment

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

@alexmt I made a couple minor comments regarding techniques and unit tests. More grandly, I have a question: it looks as though we're supporting arbitrary private keys (with the sshPrivateKeyPath option). How feasible would it be to rely solely on the local ssh-agent for stored keys, tapping through to the underlying OS? It might simplify the architecture slightly. Meanwhile, the whole thing looks good.

@alexmt
Copy link
Collaborator Author

alexmt commented Mar 15, 2018

Thank you for review @merenbach! Added missing unit tests.

I think using ssh-agent is not bad idea, but it would require storing ssh-agent files in addition to list of registered repos. It would probably require more work then manually pointing git to the appropriate key.

@alexmt alexmt merged commit 611b0e4 into argoproj:master Mar 15, 2018
@alexmt alexmt deleted the 34-ssh-git-urls branch March 15, 2018 20:00
alexec pushed a commit that referenced this pull request Apr 24, 2019
leoluz pushed a commit to leoluz/argo-cd that referenced this pull request Sep 29, 2023
* feat: implement gitops-agent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support ssh git URLs and ssh key authentication

2 participants