Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Address TODO in PR #4652 #4856

Merged
merged 2 commits into from
Nov 8, 2022
Merged

Address TODO in PR #4652 #4856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08d1e96
Address TODO in PR #4652
annasong20 Nov 4, 2022
467b837
Improve readability
annasong20 Nov 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 28 additions & 27 deletions api/loader/fileloader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ import (
"sigs.k8s.io/kustomize/kyaml/filesys"
)

// HasRemoteFileScheme returns whether path has a url scheme that kustomize allows for
// IsRemoteFile returns whether path has a url scheme that kustomize allows for
// remote files. See https://github.com/kubernetes-sigs/kustomize/blob/master/examples/remoteBuild.md
func HasRemoteFileScheme(path string) bool {
func IsRemoteFile(path string) bool {
u, err := url.Parse(path)
return err == nil && (u.Scheme == "http" || u.Scheme == "https")
}
Expand Down Expand Up @@ -299,31 +299,8 @@ func (fl *fileLoader) errIfRepoCycle(newRepoSpec *git.RepoSpec) error {
// else an error. Relative paths are taken relative
// to the root.
func (fl *fileLoader) Load(path string) ([]byte, error) {
// TODO(annasong): replace check with HasRemoteFileScheme
if u, err := url.Parse(path); err == nil && (u.Scheme == "http" || u.Scheme == "https") {
var hc *http.Client
if fl.http != nil {
hc = fl.http
} else {
hc = &http.Client{}
}
resp, err := hc.Get(path)
if err != nil {
return nil, err
}
defer resp.Body.Close()
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))
}
body, err := io.ReadAll(resp.Body)
if err != nil {
return nil, err
}
return body, nil
if IsRemoteFile(path) {
return fl.httpClientGetContent(path)
}
if !filepath.IsAbs(path) {
path = fl.root.Join(path)
Expand All @@ -335,6 +312,30 @@ func (fl *fileLoader) Load(path string) ([]byte, error) {
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)
if err != nil {
return nil, errors.Wrap(err)
}
defer resp.Body.Close()
// response unsuccessful
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))
}
content, err := io.ReadAll(resp.Body)
return content, errors.Wrap(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linter complains if I don't wrap error

Copy link
Contributor

@yuwenma yuwenma Nov 8, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@annasong20 annasong20 Nov 8, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@yuwenma yuwenma Nov 8, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, kustomize go linter enables wrapcheck.

}

// Cleanup runs the cleaner.
func (fl *fileLoader) Cleanup() error {
return fl.cleaner()
Expand Down
4 changes: 2 additions & 2 deletions api/loader/fileloader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"sigs.k8s.io/kustomize/kyaml/filesys"
)

func TestHasRemoteFileScheme(t *testing.T) {
func TestIsRemoteFile(t *testing.T) {
cases := map[string]struct {
url string
valid bool
Expand Down Expand Up @@ -50,7 +50,7 @@ func TestHasRemoteFileScheme(t *testing.T) {
for name, test := range cases {
test := test
t.Run(name, func(t *testing.T) {
require.Equal(t, test.valid, HasRemoteFileScheme(test.url))
require.Equal(t, test.valid, IsRemoteFile(test.url))
})
}
}
Expand Down