Prevent deferred File.Close and wrap errors while putting CRDs into xpls provider package cache #343
Conversation
internal/xpkg/dep/cache/entry.go
Outdated
| if fb == 0 { | ||
| return stats, errors.New(errFailedToCreateCRD) | ||
| if err := os.WriteFile(filepath.Join(e.location(), fmt.Sprintf(crdNameFmt, name)), yb, 0o600); err != nil { | ||
| return stats, errors.Wrap(err, errFailedToCreateCRD) |
There was a problem hiding this comment.
Have you double checked that the filename is part of the errors from os.WriteFile?
There was a problem hiding this comment.
Hi @sttts,
Thanks. I think the file being processed is not always part of the error message coming from os.WriteFile. In my case, for instance, it was just too many open files. Added the cache entry's path into the error's context.
internal/xpkg/dep/cache/entry.go
Outdated
| } | ||
|
|
||
| if err := e.appendToPackageJSON(jb); err != nil { | ||
| return stats, errors.New(errFailedToCreateCRD) | ||
| return stats, errors.Wrap(err, errFailedToCreateCRD) |
There was a problem hiding this comment.
that error is failed to create crd. Can a user understand what it means?
There was a problem hiding this comment.
Done. I've replaced that error message as failed to create a cache entry for the CRD at path: %s. I did not include the CRD name in the error message as the filename of the cache entry already has that information.
internal/xpkg/dep/cache/entry.go
Outdated
|
|
||
| if fb == 0 { | ||
| return stats, errors.New(errFailedToCreateCRD) | ||
| if err := os.WriteFile(filepath.Join(e.location(), fmt.Sprintf(crdNameFmt, name)), yb, 0o600); err != nil { |
There was a problem hiding this comment.
I think os.Writefile is going to side step the use of afero.Fs here. We can fix that up by using afero's API xref https://github.com/spf13/afero#using-aferos-utility-functions. So something like this:
| if err := os.WriteFile(filepath.Join(e.location(), fmt.Sprintf(crdNameFmt, name)), yb, 0o600); err != nil { | |
| if err := afero.WriteFile(e.fs, filepath.Join(e.location(), fmt.Sprintf(crdNameFmt, name)), yb, 0o600); err != nil { |
may work. Here's another example of doing that in this repo https://github.com/upbound/up/blob/main/internal/xpkg/build_test.go#L171.
There was a problem hiding this comment.
Hi @tnthornton,
Thanks, done.
ulucinar
force-pushed
the
xpkg-dep-cache
branch
2 times, most recently
from
77cca22
to
b7ffca2
Compare
internal/xpkg/dep/cache/entry.go
Outdated
| errNoObjectsToFlushToDisk = "no objects to flush" | ||
| errFailedToCreateMeta = "failed to create meta file in entry" | ||
| errFailedToCreateImageMeta = "failed to create image meta entry" | ||
| errFailedToCreateCacheEntry = "failed to create a cache entry for the CRD at path: %s" |
There was a problem hiding this comment.
nit: I believe to have seen a Fmt suffix in most places if it is a format.
There was a problem hiding this comment.
Done. Yes, that's the convention. Thank you for pointing that out.
internal/xpkg/dep/cache/entry.go
Outdated
| errFailedToCreateMeta = "failed to create meta file in entry" | ||
| errFailedToCreateImageMeta = "failed to create image meta entry" | ||
| errFailedToCreateCacheEntry = "failed to create a cache entry for the CRD at path: %s" | ||
| errFailedToAppendCRD = "failed to add the CRD to the package: %s" |
There was a problem hiding this comment.
this won't look good with errors.Wrapf: failed to add the CRD to the package: package-name: some error. Would rather use:
| errFailedToAppendCRD = "failed to add the CRD to the package: %s" | |
| errFailedToAppendCRD = "failed to add the CRD to package %q" |
…ls provider package cache Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
|
🚢 |
ulucinar
changed the title
Description of your changes
Removes the deferred
File.Closecall incache.entry.writeObjectsto decrease the # of simultaneously open files and wraps the error returned fromcache.entry.appendToPackageJSONto provide more context.I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR, as appropriate.How has this code been tested
Tested with Go 1.18.10 on a test configuration package with the following command:
(I've initially observed this issue with
up@v.17.0, which uses Go1.18)