-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: git interactor for remote instance registration #125
Conversation
* fix: validate target manager config * refactor: split config validation in smaller methods * chore: use errors instead of ok booleans
* refactor: generalize Gitlab interface to remote interactor interface * refactor: mv gitlab config to interactor config * chore: adjust imports * feat: add interactor type * feat: adjust target manager validation for interactor type * chore: adjust naming
* feat: implement remote interactor interface with git client * feat: add remote interactions * feat: add git config to interactor config * feat: validate git interactor type * chore: hide remote interactions behind remote operations interface * test: add git client unit tests * test: add git interactions unit tests
func (g *client) pushChanges(ctx context.Context) error { | ||
return g.repo.remote.PushContext(ctx, g.repo, &git.PushOptions{ | ||
Auth: g.auth, | ||
Force: true, |
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.
Is there a reason for the force push? Iirc there is one file per sparrow, so there should be no conflicts
I discussed whether we should merge this now with @lvlcn-t. We decided that it's best to leave this out-of-scope for now, as it is not required currently and to make future changes easier. We will keep the code around in case we need to implement the git integration later on |
I'll still address your reviews. After the PR would be ready to merge I'll close it, and move the branch into my fork so it won't be accidentally deleted. Additional to that I'll close the issue as not planned for now and label it with |
Motivation
We currently only support gitlab as remote registration backend through its API. This should change to support all git based repositories. For more information on that please refer to the issue.
Closes #66
Changes
I've introduced a new remote interactor type, that is able to perform our defined interactor interfaces' actions on any git repository. Currently the git interactor is only able to communicate via HTTP, but that could be changed in a future PR if a user needs ssh communication.
As you've already seen with my last two PRs regarding the issue, I've tried to do it as simple as possible so I hope the code is easy to understand. 😅 I'm also sorry that it's still ~1,5k lines changed but I think that was inevitable.
For additional information look at the commits.
Tests done
I've provided unit tests for the interactor. To do that I've done several things like hiding the remote interactions (Clone, Pull, Push) behind an interface so we can mock it and setup a
repomock
package that sets up a new local repository where the local git interactions are performed. I've consciously decided not to hide the whole go-git library behind an interface because it'd not bring us that much more benefit, but if you think we should still be able to test error cases of local git actions then we'd need to create an interface for it.Manual e2e tests
Config
Logs
Remote Git Repository
You can find the commit history of the repository here.
TODO