-
Notifications
You must be signed in to change notification settings - Fork 187
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
Refactor Git operations and introduce go-git
support for Azure DevOps and AWS CodeCommit
#944
Conversation
Signed-off-by: Sanskar Jaiswal <[email protected]>
Signed-off-by: Sanskar Jaiswal <[email protected]>
The new version uses libgit2 1.5.0 and requires git2go/v34. Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Sanskar Jaiswal <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
github.com/fluxcd/pkg/git/gogit v0.0.0-20221026111216-11a3405b2580 | ||
github.com/fluxcd/pkg/git/libgit2 v0.0.0-20221026111216-11a3405b2580 |
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 was thinking on keeping commit pinned, as there may be some follow-up changes. But then we tag and update them soon before releasing this version. Any thoughts on the approach?
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.
If we tag them before the next SC release, I'm Ok with 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.
LGTM
Thanks @pjbgf and @aryan9600 for all the hard work on the Git refactoring 🥇
go-git
support for Azure DevOps and AWS CodeCommit
Waiting for @darkowlzz's review before merging it. |
Users keen on testing source-controller with the git-refactoring changes can use the image: |
95a6360
to
6d97d09
Compare
This ensures that the event, notification and log are configured correctly. Signed-off-by: Paulo Gomes <[email protected]>
6d97d09
to
e87997c
Compare
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!
Because other will ask, can we add Gitea and Bitbucket. |
This should support all Git servers we know of. More information on the tested servers can be found at fluxcd/go-git#4. |
Err: fmt.Errorf("failed to configure checkout strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err), | ||
Reason: sourcev1.GitOperationFailedReason, | ||
} | ||
e := serror.NewStalling( |
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.
While reviewing #945, I noticed that this stalling error may not be appropriate for the above operation. The old code had stalling error because CheckoutStrategyForImplementation()
failure actually meant that retrying will not make any change. But for the above, new git client creation can fail due to various reasons and a subsequent retry may succeed.
It may be better to make sure it's an invalid git implementation error before returning stalling error. For other error while creating the git client, a generic error which results in a retry may be better.
Changes of note:
fluxcd/pkg/git
.multi_ack
andmulti_ack_detailed
, enabling support to Azure DevOps when usinggo-git
.Relates to fluxcd/pkg#245