-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Replace git cloner with go getter to support various target #2169
Conversation
Originally we used go-getter to support the remote targets. When we integrated kustomize into kubectl, the go-getter and it dependencies blocked the integration. So we removed it. There is no plan for adding it back for now. You can use the go-getter plugin instead. Thank you. |
TIL Shall we consider making the cloner configurable? i.e. instead of hardcoding it as |
cc @howardjohn for visibility |
@yujunz I think this is what @Liujingfang1 was suggesting by using a generator plugin. |
Using go-getter would be nice, we should revisit if this is possible. IIRC, the go-getter code pulled in a massive transitive dependency list. There may be a way to prune this through an upstream contribution. Or perhaps it won't be as controversial now. |
I did implement a generator plugin for go-getter. It was not so easy to configure and use. The most user friendly way to solve the issue would be include Customizable cloner still requires user to install the cloner (git, go-getter or anything) by themselves. A tradeoff for flexibility. If it looks good for all, I can make some time to implement it. |
@yujunz I'm not sure I understand the proposal? Is it to create a new pluggable-cloner mechanism that would exec out to a cloner? Or something else? |
You can take a look at the api/go.sum to check what packages have been added when you add a new dependency. For example, |
kustomize/api/internal/git/cloner.go Line 34 in 5707962
The existing cloner is a simply exec of gitProgram. So if we can provide a way to make it declarative then it should work with arbitrary binary. e.g. in
Then we shall try exec |
IIUC, we want to be cloud agnostic. I think the most wanted source is https archive. The go standard package should be sufficient. I can try to tail go-getter to make it embeddable in kustomize. Does that work for you? @Liujingfang1 |
It would be great if you can do it by go standard packages. |
b40efba
to
f3fef38
Compare
d41a0d6
to
4b97fba
Compare
@Liujingfang1 I made a try to embed the go-getter package. The changes to original go-getter are
Some additional notes
Let me know if it works for you. cc @pwittrock |
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.
Can you add some tests that are not supported previously, but work with this change?
Among the 180 files, lots of them are not needed. Can you do a clean up? I'm also wondering if you can push this go-getter version to another branch of the upstream go-getter.
@monopole Any suggestions?
api/go.mod
Outdated
@@ -16,3 +23,5 @@ require ( | |||
k8s.io/kube-openapi v0.0.0-20191107075043-30be4d16710a | |||
sigs.k8s.io/yaml v1.1.0 | |||
) | |||
|
|||
replace sigs.k8s.io/kustomize/api/internal/getter/helper/url => ./helper/url |
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.
Why adding this replace
? I don't think it's needed.
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.
removed
return newLoaderAtGitClone( | ||
repoSpec, fSys, nil, git.ClonerUsingGitExec) | ||
|
||
root, errD := demandDirectoryRoot(fSys, target) |
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.
ditto
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.
ditto
if err == nil { | ||
// Treat this as git repo clone request. | ||
|
||
root, errD := func() (filesys.ConfirmedDir, error) { |
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 previous order is first to treat target as a remote target; when failed, treat it as a local target. The changes make the order reverted. Is there any reason for doing this?
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.
In fact, getter is capable to handle local target so we cannot check it with the error return as repoSpec, err := git.NewRepoSpecFromUrl(path)
. However it may not behavior exactly the same, e.g. the containment violation.
Putting local target before getter is to ensure backward compatibility.
You mean an integration test? We can update the examples to include them, which is also covered by automatic testing IIRC. For unit test, I think it is self contained. We may mock some call to ensure it reaches the right method.
I can try my best to clean up a bit.
Not sure it can be accepted but we definitely can propose that. Any recommended justification? |
Pushed an update with dependency on a forked go-getter. It is quite tricky to handle Do you think we can host a fork under kubernetes-sigs organization? That would make the patch in kustomize cleaner and easier to maintain. @Liujingfang1 @monopole |
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.
Replies inline.
Link upstream discussion about reducing dependency: hashicorp/go-getter#193 |
Let's merge this for now. When we can get some updates from go-getter side for hashicorp/go-getter#193, we can change the dependency to the lite version of go-getter. Thank you for working on this. @yujunz As a follow up, can you add an integration test or an example test for this? /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Liujingfang1, yujunz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Revert "Merge pull request #2169 from yujunz/loader/go-getter"
Fix #923 #1272