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.

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

Localize resources #4912

Merged
merged 4 commits into from
Jan 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions api/internal/localizer/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,18 @@ type ResourceLoadError struct {
FileError error
}

var _ error = ResourceLoadError{}

func (rle ResourceLoadError) Error() string {
return fmt.Sprintf(`when parsing as inline received error: %s
when parsing as filepath received error: %s`, rle.InlineError, rle.FileError)
}

type PathLocalizeError struct {
Path string
FileError error
RootError error
}

func (ple PathLocalizeError) Error() string {
return fmt.Sprintf(`could not localize path %q as file: %s; could not localize path %q as directory: %s`,
ple.Path, ple.FileError, ple.Path, ple.RootError)
}
51 changes: 44 additions & 7 deletions api/internal/localizer/localizer.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,15 @@ func (lc *localizer) localizeNativeFields(kust *types.Kustomization) error {
kust.Crds,
lc.localizeFile,
},
"resources": {
kust.Resources,
lc.localizeResource,
},
} {
for i, path := range field.paths {
locPath, err := field.locFn(path)
if err != nil {
return errors.WrapPrefixf(err, "unable to localize %s path", fieldName)
return errors.WrapPrefixf(err, "unable to localize %s entry", fieldName)
}
field.paths[i] = locPath
}
Expand Down Expand Up @@ -198,8 +202,6 @@ func (lc *localizer) localizeNativeFields(kust *types.Kustomization) error {
kust.Replacements[i].Path = newPath
}
}

// TODO(annasong): localize all other kustomization fields: resources
return nil
}

Expand Down Expand Up @@ -254,6 +256,39 @@ func (lc *localizer) localizePatches(patches []types.Patch) error {
return nil
}

// localizeResource localizes resource path, a file or root, and returns the
// localized path
func (lc *localizer) localizeResource(path string) (string, error) {
var locPath string

content, fileErr := lc.ldr.Load(path)
// The following check catches the case where path is a repo root.
// Load on a repo will successfully return its README in HTML.
// Because HTML does not follow resource formatting, we then correctly try
// to localize path as a root.
if fileErr == nil {
_, resErr := lc.rFactory.NewResMapFromBytes(content)
if resErr != nil {
fileErr = errors.WrapPrefixf(resErr, "invalid resource at file %q", path)
} else {
locPath, fileErr = lc.localizeFileWithContent(path, content)
}
}
if fileErr != nil {
var rootErr error
locPath, rootErr = lc.localizeDir(path)
if rootErr != nil {
err := PathLocalizeError{
Path: path,
FileError: fileErr,
RootError: rootErr,
}
return "", err
}
}
return locPath, nil
}

// localizeFile localizes file path and returns the localized path
func (lc *localizer) localizeFile(path string) (string, error) {
content, err := lc.ldr.Load(path)
Expand All @@ -267,8 +302,9 @@ func (lc *localizer) localizeFile(path string) (string, error) {
func (lc *localizer) localizeFileWithContent(path string, content []byte) (string, error) {
var locPath string
if loader.IsRemoteFile(path) {
// TODO(annasong): You need to check if you can add a localize directory here to store
// the remote file content. There may be a directory that shares the localize directory name.
if lc.fSys.Exists(lc.root.Join(LocalizeDir)) {
return "", errors.Errorf("%s already contains %s needed to store file %q", lc.root, LocalizeDir, path)
}
locPath = locFilePath(path)
} else {
// ldr has checked that path must be relative; this is subject to change in beta.
Expand Down Expand Up @@ -306,8 +342,9 @@ func (lc *localizer) localizeDir(path string) (string, error) {
}
var locPath string
if repo := ldr.Repo(); repo != "" {
// TODO(annasong): You need to check if you can add a localize directory here to store
// the remote file content. There may be a directory that shares the localize directory name.
if lc.fSys.Exists(lc.root.Join(LocalizeDir)) {
return "", errors.Errorf("%s already contains %s needed to store root %q", lc.root, LocalizeDir, path)
}
locPath, err = locRootPath(path, repo, root, lc.fSys)
if err != nil {
return "", err
Expand Down
50 changes: 50 additions & 0 deletions api/internal/localizer/localizer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1013,3 +1013,53 @@ nameSuffix: -test
}
checkLocalizeInTargetSuccess(t, kustAndComponents)
}

func TestLocalizeResources(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a tests here with a remote resource file, and a remote resource directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't add that test yet because the code that calculates the path to save a remote root, #4909, hasn't been merged yet.

Also, since these are integration tests, I planned to add them in a separate PR in krusty. I can add them in krusty in this PR once #4909 is merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that the localizeFile and localizeDir functions are supposed to be converting the remote URLs to local filepaths, so IMO these tests should cover that behavior. Can we get #4909 in and then add the tests here?

Copy link
Contributor

@natasha41575 natasha41575 Dec 9, 2022

Choose a reason for hiding this comment

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

I'm also OK to merge this before #4909, if we add these tests in #4909. Let me know which you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that the localizeFile and localizeDir functions are supposed to be converting the remote URLs to local filepaths, so IMO these tests should cover that behavior. Can we get #4909 in and then add the tests here?

Sure, sg.

kustAndResources := map[string]string{
"kustomization.yaml": `apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- pod.yaml
- ../../alpha
`,
"pod.yaml": podConfiguration,
"../../alpha/kustomization.yaml": `apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
namePrefix: my-
`,
}
expected, actual := makeFileSystems(t, "/a/b", kustAndResources)

err := Run("/a/b", "/", "", actual)
require.NoError(t, err)

addFiles(t, expected, "/localized-b/a/b", kustAndResources)
checkFSys(t, expected, actual)
}

func TestLocalizePathError(t *testing.T) {
kustAndResources := map[string]string{
"kustomization.yaml": `apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- b
`,
}
expected, actual := makeFileSystems(t, "/a", kustAndResources)

err := Run("/a", "/", "", actual)

const expectedFileErr = `invalid file reference: '/a/b' must resolve to a file`
const expectedRootErr = `unable to localize root "b": unable to find one of 'kustomization.yaml', 'kustomization.yml' or 'Kustomization' in directory '/a/b'`
var actualErr PathLocalizeError
require.ErrorAs(t, err, &actualErr)
require.Equal(t, "b", actualErr.Path)
require.EqualError(t, actualErr.FileError, expectedFileErr)
require.EqualError(t, actualErr.RootError, expectedRootErr)

const expectedErrPrefix = `unable to localize target "/a": unable to localize resources entry`
require.EqualError(t, err, fmt.Sprintf(`%s: could not localize path "b" as file: %s; could not localize path "b" as directory: %s`,
expectedErrPrefix, expectedFileErr, expectedRootErr))

checkFSys(t, expected, actual)
}
Loading