Address TODO in PR #4652

[annasong20]

The HasRemoteFileScheme() function was introduced in #4652, but its use in fileloader.go was reverted due to the #4756 code freeze. This PR reinstates its usage.

[annasong20]

@yuwenma, @natasha41575

[k8s-ci-robot]

@annasong20: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[annasong20]

/label tide/merge-method-squash

[yuwenma]

For "too many nested condition" linter error, a proper fix is normally about finding out the root cause and improve the readability (e.g. this doc gives some examples). Moving to another function is more or less a workaround.

When looking at the Load function, a clearer logic would be

• if path is remote, fetch remote content
• if path is relative path, do ....
• if path is absolute path, do ...

I would change the code as

func (fl *fileLoader) Load(path string) ([]byte, error) {
	if isRemote(path) {
		return fl.httpClientGetContent(path)
	}
	if !filepath.IsAbs(path) {
		path = fl.root.Join(path)
	}
	path, err := fl.loadRestrictor(fl.fSys, fl.root, path)
	if err != nil {
		return nil, err
	}
	return fl.fSys.ReadFile(path)
}

func (fl *fileLoader) httpClientGetContent(path string) ([]byte, error) {
	var hc *http.Client
	if fl.http != nil {
		hc = fl.http
	} else {
		hc = &http.Client{}
	}
	resp, err := hc.Get(path)
	defer resp.Body.Close()
	// comment explaining what error are not accepted
	if resp.StatusCode < 200 || resp.StatusCode > 299 {
		_, err := git.NewRepoSpecFromURL(path)
		if err == nil {
			return nil, errors.Errorf("URL is a git repository")
		}
		return nil, fmt.Errorf("%w: status code %d (%s)", ErrHTTP, resp.StatusCode, http.StatusText(resp.StatusCode))
	}
	return io.ReadAll(resp.Body)
}

func isRemote(path string) bool {
	u, err := url.Parse(path)
	if err != nil {
		return false
	}
	return u.Scheme == "http" || u.Scheme == "https"
}

[yuwenma]

I'm okay if you want to make the change in a follow-up PR. This is not a blocker of the kustomize localize. Approved!

[annasong20]

linter complains if I don't wrap error

[yuwenma]

hmmm, what kind of linter error? can you return the ReadAll directly (merge ln335-336)?
return io.ReadAll(resp.Body)

[annasong20]

No, I don't believe I can. I get the following linter error:

api/loader/fileloader.go:335:9: error returned from external package is unwrapped: sig: func io.ReadAll(r io.Reader) ([]byte, error) (wrapcheck)
	return io.ReadAll(resp.Body)
	       ^
make: *** [lint] Error 1

I can wrap the error with a helpful message to make the wrap more meaningful?

[yuwenma]

Oh I see, kustomize go linter enables wrapcheck.

[annasong20]

I'm okay if you want to make the change in a follow-up PR. This is not a blocker of the kustomize localize. Approved!

Addressed comment. Thank you so much for the code example!

linter currently failing due to previous change to master; #4854 should fix this, so I won't do anything here

[yuwenma]

Looks good to me.
Can you run go mod tidy to fix the CI error?

[natasha41575]

Deleted an above comment because it was no longer relevant (I think I had an old version of this PR up).

/approve

Will let @yuwenma give the final LGTM.

[yuwenma]

hmmm, what kind of linter error? can you return the ReadAll directly (merge ln335-336)?
return io.ReadAll(resp.Body)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: annasong20, natasha41575, yuwenma

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 [natasha41575]

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

[yuwenma]

Approved! @annasong20
one nit comment. please resolve before merging.

[yuwenma]

/lgtm