Support various target and resource with go-getter #2278

yujunz · 2020-03-15T12:04:27Z

getter is inserted before git cloner
local file is NOT handled by getter
tested against existing examples

yujunz · 2020-03-15T12:05:53Z

Backward compatible with existing examples

hack/testExamplesAgainstKustomize.sh HEAD
Example tests passed against HEAD.

yujunz · 2020-03-15T12:19:22Z

/test kustomize-presubmit-master

yujunz · 2020-03-15T12:24:40Z

Unable to understand the lint error:

level=error msg="Running error: SA2003: failed prerequisites: [[email protected]/kustomize/api/resmap_test [sigs.k8s.io/kustomize/api/resmap.test]: analysis skipped: errors in package: [/home/prow/go/src/sigs.k8s.io/kustomize/api/resmap/resmap_test.go:730:40: cannot use rf (variable of type *resource.Factory) as *resource.Factory value in argument to resmaptest_test.NewRmBuilder

Is it a flake? I didn't even change the file.

yujunz · 2020-03-19T02:53:39Z

Ready for review @Liujingfang1 @pwittrock

Liujingfang1 · 2020-03-19T16:18:37Z

@yujunz Where did you see the lint error? Is it resolved?

Liujingfang1 · 2020-03-19T17:17:26Z

api/loader/fileloader.go

 }

 // newLoaderAtConfirmedDir returns a new fileLoader with given root.
 func newLoaderAtConfirmedDir(
 	lr LoadRestrictorFunc,
 	root filesys.ConfirmedDir, fSys filesys.FileSystem,
-	referrer *fileLoader, cloner git.Cloner) *fileLoader {
+	referrer *fileLoader, cloner git.Cloner, getter resourceGetter) *fileLoader {


I looks like getter and cover the functionality of cloner. Can you see if we can use getter only rather than both?

There is an intersection between getter and cloner. Some special URLs can only be handled by cloner at the moment.

One thing we can do is refactoring cloner to Detect and Get in getter and register them in getter. Then we shall have the unified interface while keeping backward compatibility.

What do you think?

What are the special URLs that can't be handled by cloner? If they are not supported by getter, keeping the cloner for them should be fine.

For example,

kustomize/api/internal/git/repospec.go

Line 51 in 064f064

if isAzureHost(x.Host) || isAWSHost(x.Host) {

I don't see detectors in go-getter to handle them. It supports only plain git, GitHub and bitbucket url.

yujunz · 2020-03-20T01:49:51Z

Where did you see the lint error? Is it resolved?

Yes, it has been resolved by #2280

Liujingfang1 · 2020-03-23T16:14:08Z

api/loader/resource.go

+// Getter is a function that can gets resource
+type resourceGetter func(rs *resourceSpec) error
+
+func newResourceGetter(raw string, fSys filesys.FileSystem, referrer *fileLoader, cloner git.Cloner, getter resourceGetter) (ifc.Loader, error) {


Change the function name of newResourceGetter. Maybe newLoaderAtGetter to be consistent with other function names.

Liujingfang1 · 2020-03-23T16:16:19Z

examples/remoteBuild.md


-  `github.com/kubernetes-sigs/kustomize/examples/multibases?ref=v1.0.6`
- a subdirectory in a repo on branch repoUrl2
+# a repo with a root level kustomization.yaml on branch test


How about github.com/kubernetes-sigs/kustomize/examples/multibases?ref=v1.0.6?

This is already covered by L91

# a subdirectory in a repo on branch repoUrl2 - github.com/Liujingfang1/kustomize/examples/helloWorld?ref=repoUrl2

Already covered in L91 that followed. kustomize repository is too huge for testing. It takes a long time to clone when network is not so good.

Liujingfang1 · 2020-03-23T16:19:55Z

api/loader/resource.go

+		return nil, err
+	}
+
+	// TODO(yujunz): check file or directory


What do you mean by this todo?

A resource can either be a plain yaml or an archive. The former should be loaded as it is and the latter needs to be extracted to directory. It turns out this is already handled in

kustomize/api/internal/target/kusttarget.go

Line 304 in 3a2635b

// try loading resource as file then as base (directory or git repository)

Removed the TODO item

Liujingfang1 · 2020-03-23T16:20:47Z

api/loader/resource.go

+	// TODO(yujunz): check file or directory
+
+	return &fileLoader{
+		loadRestrictor: RestrictionRootOnly,


The loadRestrictor should be inherited from the parent overlay.

This is the top level similar tonewLoaderAtGiClone

kustomize/api/loader/fileloader.go

Line 226 in 3a2635b

// Clones never allowed to escape root.

Yes, you're correct. For the remote target, we should restrict the load restrictor to be within that remote target.

Liujingfang1 · 2020-03-23T16:23:40Z

api/loader/resource.go

+}
+
+// Getter is a function that can gets resource
+type resourceGetter func(rs *resourceSpec) error


Suggest to change resourceGetter to remoteTargetGetter since resource has a specific meaning in kustomize, which is different from the usage here.

Liujingfang1 · 2020-03-23T16:24:01Z

api/loader/resource.go

+	"sigs.k8s.io/kustomize/api/internal/git"
+)
+
+type resourceSpec struct {


resourceSpec -> remoteTargetSpec

yujunz

Comments inline

Liujingfang1 · 2020-03-24T16:13:06Z

/lgtm
/approve

k8s-ci-robot · 2020-03-24T16:13:14Z

[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:

~~OWNERS~~ [Liujingfang1]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 15, 2020

k8s-ci-robot requested review from Liujingfang1 and pwittrock March 15, 2020 12:04

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 16, 2020

yujunz force-pushed the getter branch 2 times, most recently from 8c2554e to ea5d780 Compare March 16, 2020 08:59

yujunz mentioned this pull request Mar 16, 2020

Exlude golangci-lint v1.24.0 which breaks CI #2280

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2020

yujunz force-pushed the getter branch from ea5d780 to db1f6f3 Compare March 19, 2020 02:21

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 19, 2020

Liujingfang1 reviewed Mar 19, 2020

View reviewed changes

Liujingfang1 reviewed Mar 23, 2020

View reviewed changes

Support various target and resource with go-getter

51b29d7

yujunz commented Mar 24, 2020

View reviewed changes

yujunz force-pushed the getter branch from db1f6f3 to a0e7abb Compare March 24, 2020 02:12

Rename resourceGetter to remoteTargetGetter for readability

04bec8e

yujunz force-pushed the getter branch from a0e7abb to 04bec8e Compare March 24, 2020 02:20

k8s-ci-robot assigned Liujingfang1 Mar 24, 2020

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 24, 2020

k8s-ci-robot merged commit cd6614a into kubernetes-sigs:master Mar 24, 2020

yujunz deleted the getter branch March 25, 2020 01:40

yujunz mentioned this pull request Mar 28, 2020

Feature Request: Move git cloner to to go-git #2281

Closed

yujunz mentioned this pull request Nov 18, 2020

kustomize 3.5.5 breaks installing ArgoCD from URL #2538

Closed

monopole mentioned this pull request Feb 12, 2021

The resource loader requires too many deps. #3578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support various target and resource with go-getter #2278

Support various target and resource with go-getter #2278

yujunz commented Mar 15, 2020 •

edited

Loading

yujunz commented Mar 15, 2020

yujunz commented Mar 15, 2020

yujunz commented Mar 15, 2020

yujunz commented Mar 19, 2020

Liujingfang1 commented Mar 19, 2020

Liujingfang1 Mar 19, 2020

yujunz Mar 20, 2020

Liujingfang1 Mar 20, 2020

yujunz Mar 21, 2020

yujunz commented Mar 20, 2020

Liujingfang1 Mar 23, 2020

yujunz Mar 24, 2020

Liujingfang1 Mar 23, 2020

yujunz Mar 24, 2020

yujunz Mar 24, 2020

Liujingfang1 Mar 23, 2020

yujunz Mar 24, 2020

Liujingfang1 Mar 23, 2020

yujunz Mar 24, 2020

Liujingfang1 Mar 24, 2020

Liujingfang1 Mar 23, 2020

yujunz Mar 24, 2020

Liujingfang1 Mar 23, 2020

yujunz Mar 24, 2020

yujunz left a comment

Liujingfang1 commented Mar 24, 2020

k8s-ci-robot commented Mar 24, 2020

Support various target and resource with go-getter #2278

Support various target and resource with go-getter #2278

Conversation

yujunz commented Mar 15, 2020 • edited Loading

yujunz commented Mar 15, 2020

yujunz commented Mar 15, 2020

yujunz commented Mar 15, 2020

yujunz commented Mar 19, 2020

Liujingfang1 commented Mar 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujunz commented Mar 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujunz left a comment

Choose a reason for hiding this comment

Liujingfang1 commented Mar 24, 2020

k8s-ci-robot commented Mar 24, 2020

yujunz commented Mar 15, 2020 •

edited

Loading