✨ follow-ups for containers/image from catalogd and previous PR #1270

joelanford · 2024-09-14T00:46:20Z

Description

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-09-14T00:46:37Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`eb59925`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/66f69d1a62aa3600089de30d
😎 Deploy Preview	https://deploy-preview-1270--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-09-14T00:53:09Z

Codecov Report

Attention: Patch coverage is 69.31818% with 27 lines in your changes missing coverage. Please review.

Project coverage is 75.81%. Comparing base (8699d25) to head (eb59925).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/rukpak/source/containers_image.go	67.85%	19 Missing and 8 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1270      +/-   ##
==========================================
- Coverage   76.07%   75.81%   -0.26%     
==========================================
  Files          40       40              
  Lines        2378     2431      +53     
==========================================
+ Hits         1809     1843      +34     
- Misses        401      414      +13     
- Partials      168      174       +6

Flag	Coverage Δ
e2e	`58.24% <52.27%> (-0.25%)`	⬇️
unit	`52.28% <61.36%> (+0.18%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tmshort · 2024-09-19T14:51:52Z

internal/rukpak/source/containers_image.go

@@ -89,7 +85,11 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour
 if err != nil {
 return nil, fmt.Errorf("error creating temporary directory: %w", err)
 }
- defer os.RemoveAll(layoutDir)
+ defer func() {
+ if err := os.RemoveAll(layoutDir); err != nil {


Should this be deleteRecursive()?

This one doesn't have to be because layoutDir only ever exists while this function is running, and we never set it to be read-only like we do for the unpack dir.

tmshort · 2024-09-19T14:53:19Z

internal/rukpak/source/containers_image.go

+ return imgRef, canonicalRef, isCanonical, nil
+}
+
+func resolveCanonicalRef(ctx context.Context, imgRef reference.Named, imageCtx *types.SystemContext) (reference.Canonical, bool, error) {


It's not clear what the bool return is for. There's no explanation (I assume it's isCanonical?)

Yeah, its isCanonical. One way to tell is by looking at the call site in line 178.

But, this one is a bit weird because operator-controller doesn't actually use this variable. Keep moving up the stack and you'll see we discard its value on line 49.

As best I can, I'm trying to keep the function signatures the same between catalogd and operator-controller to facilitate merging later.

tmshort · 2024-09-19T14:54:32Z

internal/rukpak/source/containers_image.go

@@ -217,7 +241,7 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
 return fmt.Errorf("error creating image source: %w", err)
 }

- if err := os.MkdirAll(unpackPath, 0755); err != nil {
+ if err := os.MkdirAll(unpackPath, 0700); err != nil {


Why the restrictive mode?

Also noting setReadOnlyRecursive... these only set the owner's bits, not group/other.

It is the same mode as we are setting in applyLayerFilter()

Just trying to be consistent every where. These files are always written and read just by us, so these permissions should be fine. The cache dir is in an emptyDir mount that starts out empty every time a pod starts, so we should never need to be reading files written by another UID.

LalatenduMohanty · 2024-09-25T21:02:23Z

internal/rukpak/source/containers_image.go

@@ -45,14 +46,9 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour
 // Resolve a canonical reference for the image.
 //
 //////////////////////////////////////////////////////
- imgRef, err := reference.ParseNamed(bundle.Image.Ref)
+ imgRef, canonicalRef, _, err := resolveReferences(ctx, bundle.Image.Ref, i.SourceContext)


@joelanford I am curious about this change. What triggered this change?

We are moving from a containers/image method to local implementation, hence the question.

Nevermind I saw that it is just a wrapper on top of reference.ParseNamed(ref)

It was based on a suggestion from catalogd's implementation PR for containers/image. I'm trying to keep them as close to the same as I can.

Signed-off-by: Joe Lanford <[email protected]>

everettraven · 2024-09-27T19:23:46Z

internal/rukpak/source/containers_image.go

+ case 0: // regular file
+ return os.Chmod(path, 0400)
+ default:
+ return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String())


Nit: Should we use the type format directive here?

Suggested change

return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String())

return fmt.Errorf("refusing to change ownership of file %q with type %T", path, typ)

No, I don't think so. This is a file type, not a Go type.

tmshort

/lgtm

I'm ok with the explanations.

joelanford requested a review from a team as a code owner September 14, 2024 00:46

joelanford force-pushed the containers-image-follow-up branch 3 times, most recently from 90cfc74 to 518c8e8 Compare September 14, 2024 18:36

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2024

joelanford force-pushed the containers-image-follow-up branch from 518c8e8 to 01ffcc0 Compare September 18, 2024 21:06

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2024

tmshort reviewed Sep 19, 2024

View reviewed changes

LalatenduMohanty reviewed Sep 25, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2024

joelanford force-pushed the containers-image-follow-up branch from 01ffcc0 to 047a9fc Compare September 27, 2024 01:19

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2024

follow-ups for containers/image from catalogd and previous PR

eb59925

Signed-off-by: Joe Lanford <[email protected]>

joelanford force-pushed the containers-image-follow-up branch from 047a9fc to eb59925 Compare September 27, 2024 11:55

everettraven reviewed Sep 27, 2024

View reviewed changes

everettraven approved these changes Sep 27, 2024

View reviewed changes

tmshort approved these changes Sep 27, 2024

View reviewed changes

openshift-ci bot assigned tmshort Sep 27, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2024

joelanford added this pull request to the merge queue Sep 27, 2024

Merged via the queue into operator-framework:main with commit f169414 Sep 27, 2024
17 of 19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ follow-ups for containers/image from catalogd and previous PR #1270

✨ follow-ups for containers/image from catalogd and previous PR #1270

joelanford commented Sep 14, 2024

netlify bot commented Sep 14, 2024 •

edited

Loading

codecov bot commented Sep 14, 2024 •

edited

Loading

tmshort Sep 19, 2024

joelanford Sep 27, 2024

tmshort Sep 19, 2024

joelanford Sep 27, 2024

tmshort Sep 19, 2024

tmshort Sep 19, 2024

LalatenduMohanty Sep 25, 2024

joelanford Sep 27, 2024

LalatenduMohanty Sep 25, 2024

LalatenduMohanty Sep 25, 2024

LalatenduMohanty Sep 25, 2024

joelanford Sep 27, 2024

everettraven Sep 27, 2024

joelanford Sep 27, 2024

tmshort left a comment

	return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String())
	return fmt.Errorf("refusing to change ownership of file %q with type %T", path, typ)

✨ follow-ups for containers/image from catalogd and previous PR #1270

✨ follow-ups for containers/image from catalogd and previous PR #1270

Conversation

joelanford commented Sep 14, 2024

Description

Reviewer Checklist

netlify bot commented Sep 14, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Sep 14, 2024 • edited Loading

Codecov Report

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

tmshort left a comment

Choose a reason for hiding this comment

netlify bot commented Sep 14, 2024 •

edited

Loading

codecov bot commented Sep 14, 2024 •

edited

Loading