-
Notifications
You must be signed in to change notification settings - Fork 29
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
#173: use force update mode for ide-urls #155
Conversation
replaced git pull with hard reset and cleanup added force param to gitPullOrClone
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.
Looks good to me :)
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.
@jan-vcapgemini thanks for your PR with analysis and fix. 👍
Great that you also addressed some code-cleanups.
I like your solution and also I think it is a very good idea that you added an explicit force
flag instead of (mis)using our --force
mode for this as this more depends on the Git use-case and for ide-urls
this is exactly desired while for ide-settings
this could end in a disaster as end-users may loose lots of valuable configuration work.
So in general I am happy with your PR and would like to merge it.
However, since you stumbled over some odd stuff in our code-base and started improving, I would love to take the opportunity to really fix that even though not related to the actual story.
cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java
Outdated
Show resolved
Hide resolved
…text.java Co-authored-by: Jörg Hohwiller <[email protected]>
…text.java Co-authored-by: Jörg Hohwiller <[email protected]>
Pull Request Test Coverage Report for Build 8002948607Details
💛 - Coveralls |
added target to warning messages added a new warning if git failed to clean the local repository
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.
@jan-vcapgemini thanks for your rework. Now all looks good. 👍
I will however have to test this manually before I will merge.
# Conflicts: # cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java
removed gitPullOrClone from IdeContext interface moved gitPullOrCloneIfNeeded to GitUtils
added and implemented new GitContext interface added getGitContext to IdeContext
removed GitUtilsProcessResultMock renamed GitUtils to GitContext
# Conflicts: # cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java
cli/src/main/java/com/devonfw/tools/ide/context/GitContext.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/context/GitContext.java
Outdated
Show resolved
Hide resolved
removed remoteName and branchName added remote and branch name retrieval added retrieveRemoteAndBranchName method to GitContextImpl
removed behind message from branch
added new GitUrl record added new pullOrFetchAndResetIfNeeded (for ide-urls only) adjusted tests (added HEAD file) passed GitUrl to clone method
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.
@jan-vcapgemini Thanks for completing this complex PR. Great job - you have reached a great state & result 👍
Ready for merge.
Implements: #173
gitPullOrClone
git pull
followed withget reset
andgit cleanup
GitUtils
class (handles git commands and error messages)Does not implement: