-
Notifications
You must be signed in to change notification settings - Fork 24
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
#312: Added ability to prefer git
(ssh) protocol instead of https
for cloning repos
#625
Conversation
… GitUrl.java to process ssh urls as well, and added tests
Pull Request Test Coverage Report for Build 11477617445Details
💛 - Coveralls |
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.
@diiinesh I already had a look even though still in draft mode.
You did already a great job with GitUrlSyntax
and the integration into GitContextImpl
plus adding a wonderful JUnit test. 👍
I already left some suggestions for improvement and refactoring early on...
Please have a look.
cli/src/main/java/com/devonfw/tools/ide/context/GitUrlSyntax.java
Outdated
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/context/GitContextTest.java
Outdated
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/context/GitContextTest.java
Outdated
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/context/GitContextTest.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/context/GitContextImpl.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Jörg Hohwiller <hohwille@users.noreply.github.com>
….java Co-authored-by: Jörg Hohwiller <hohwille@users.noreply.github.com>
…num.java and seperate IdeVariable, moved test functions to GitUrlSyntax.java
# Conflicts: # cli/src/main/java/com/devonfw/tools/ide/context/GitContextImpl.java
…project for git based implementations and its config file
…to Enhancement/git_ssh_protocol
…to Enhancement/git_ssh_protocol
…aces and home dir in test env
…to Enhancement/git_ssh_protocol
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.
LGTM
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.
@diiinesh thanks for your rework. 👍
I left some small review comments for improvement.
However, this PR does not integrate the new feature so story #312 is not yet implemented.
You need to change GitContextImpl
to actually make use of the new infrastructure that you have created in order to transform the GitUrl
before calling the actual git clone command.
Also I would suggest to add GitUrlSyntax.DEFAULT
and provide this via default lambda in IdeVariables
as default so that you can avoid NullPointerException and do not always need to check if configured GitUrlSyntax
is null
.
cli/src/main/java/com/devonfw/tools/ide/variable/VariableDefinitionEnum.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/variable/IdeVariables.java
Outdated
Show resolved
Hide resolved
…itionEnum.java Co-authored-by: Jörg Hohwiller <hohwille@users.noreply.github.com>
git
(ssh) protocol instead of https
for cloning repos git
(ssh) protocol instead of https
for cloning repos
@@ -73,6 +75,11 @@ public interface IdeVariables { | |||
/** {@link VariableDefinition} for {@link com.devonfw.tools.ide.context.IdeContext#getProjectName() PROJECT_NAME}. */ | |||
VariableDefinitionString PROJECT_NAME = new VariableDefinitionString("PROJECT_NAME", null, c -> c.getProjectName()); | |||
|
|||
/** Preferred Git protocol (HTTPS/SSH) as defined by {@link GitUrlSyntax}. */ | |||
VariableDefinitionEnum<GitUrlSyntax> PREFERRED_GIT_PROTOCOL = new VariableDefinitionEnum<>("PREFERRED_GIT_PROTOCOL", null, GitUrlSyntax.class, | |||
c -> GitUrlSyntax.HTTPS |
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.
The default should be not to change the URL. Only if the user sets the property explicitly, the URL should be changed.
See #625:
Default behavior remains unchanged if the property is not set, using the provided Git URL as is.
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.
Done in #724
This PR is replaced by PR #724 - hence closing. |
Adds #312
GIT_PREFERRED_PROTOCOL
property to allow configuration of preferred Git protocol (SSH or HTTPS).