✨ Use creds if present for pulling bundle images #1303

anik120 · 2024-09-24T14:12:01Z

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-24T14:12:18Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`160b7ca`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/66f552b9fb3ba300087c6535
😎 Deploy Preview	https://deploy-preview-1303--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-24T14:19:09Z

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 76.50%. Comparing base (62b5c53) to head (160b7ca).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/manager/main.go	55.55%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1303      +/-   ##
==========================================
- Coverage   76.60%   76.50%   -0.10%     
==========================================
  Files          40       40              
  Lines        2406     2422      +16     
==========================================
+ Hits         1843     1853      +10     
- Misses        395      401       +6     
  Partials      168      168

Flag	Coverage Δ
e2e	`58.71% <55.55%> (+0.56%)`	⬆️
unit	`52.97% <0.00%> (-0.15%)`	⬇️

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.

cmd/manager/main.go

anik120 · 2024-09-26T10:38:25Z

cmd/manager/main.go

+		logger.Error(err, "could not stat auth file path", "path", authFilePath)
+		return ""


Are we okay with not panicking here if an unknown stat error occurs here?

Yeah, you're probably right. Should we have this function return (string, error)? That makes calling it slightly more verbose to call, but then we could do the error check and have the logging/os.Exit happen in main. And at that point, no need to pass a logger in.

I like the idea of keeping the signature the same and then os.Exit(1) instead of return "". Updated the PR.

everettraven · 2024-09-26T13:10:35Z

cmd/manager/main.go

@@ -311,3 +315,15 @@ type finalizerFunc func(ctx context.Context, obj client.Object) (crfinalizer.Res
 func (f finalizerFunc) Finalize(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
 	return f(ctx, obj)
 }
+
+func authFilePathIfPresent(logger logr.Logger) string {


Nit: Could we name this something like mustAuthFilePathIfPresent or authFilePathIfPresentOrDie? I think that it would help someone reading through the code where this function is used to understand that if there is an error (unhandled error type in this case) that this function will exit the program.

I see where you're coming from.
....

not sure if I would understand what mustAuthFilePathIfPresent means if I didn't know the context....

authFilePathIfPresentOrDie sounds like it must find an auth file or panic if it's not present, which is not the case.

So I think this one's not that easy 😅

Actually, I could just leave comment on top that says unhandled error will cause is to exit with error code 1. Gets the job done.

While writing the comment I realized I have to basically summarize in English words every single thing that the function is doing, just to get to the part about the error code 1, which seems superfluous 😅

// authFilePathIfPresent returns the value of the constant // authFilePath ("/etc/catalogd/auth.json") if the file is // present, emptry string ("") otherwise, using os.Stat(filePath). // If os.Stat returns an error that's not IsNotExist, the function // causes the program to exit with error code 1

If there is no reasonable way we can come up with to convey this as part of the function name that is fine. I won't hold the PR on this nit, just thought it worthwhile to call out

MCO makes the global pull secrets available in `/var/lib/kubelet`. Operator-controller will look for these secrets in `/etc/operator-controller` folder, ref [operator-controller:1303](operator-framework/operator-controller#1303). This PR hostPath mounts the `/var/lib/kublet` directory from the host to the `/etc/operator-controller` directory in the container's filesystem.

MCO makes the global pull secrets available in `/var/lib/kubelet`. Operator-controller will look for these secrets in `/etc/operator-controller` folder, ref [operator-controller:1303](operator-framework/operator-controller#1303). This PR hostPath mounts the `/var/lib/kublet` directory from the host to the `/etc/operator-controller` directory in the container's filesystem. RFC: [OLMv1 Private registry support](https://docs.google.com/document/d/1BXD6kj5zXHcGiqvJOikU2xs8kV26TPnzEKp6n7TKD4M/edit?usp=sharing)

MCO makes the global pull secrets available in `/var/lib/kubelet`. Operator-controller will look for these secrets in `/etc/operator-controller` folder, ref [operator-controller:1303](operator-framework/operator-controller#1303). This PR hostPath mounts the `/var/lib/kublet` directory from the host to the `/etc/operator-controller` directory in the container's filesystem. RFC: [OLMv1 Private registry support](https://docs.google.com/document/d/1BXD6kj5zXHcGiqvJOikU2xs8kV26TPnzEKp6n7TKD4M/edit?usp=sharing) Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>

anik120 requested a review from a team as a code owner September 24, 2024 14:12

anik120 marked this pull request as draft September 24, 2024 14:12

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2024

joelanford reviewed Sep 25, 2024

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

joelanford reviewed Sep 25, 2024

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

anik120 force-pushed the support-private-registries branch from d2cbaf9 to e1b61aa Compare September 26, 2024 10:37

anik120 commented Sep 26, 2024

View reviewed changes

✨ Use creds if present for pulling bundle images

160b7ca

anik120 force-pushed the support-private-registries branch from e1b61aa to 160b7ca Compare September 26, 2024 12:25

everettraven reviewed Sep 26, 2024

View reviewed changes

anik120 mentioned this pull request Sep 26, 2024

OPRUN-3554: UPSTREAM: <carry>: Add hostPath mount for /var/lib/kubelet openshift/operator-framework-operator-controller#152

Merged

anik120 marked this pull request as ready for review September 26, 2024 15:08

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 26, 2024

anik120 self-assigned this Sep 26, 2024

joelanford approved these changes Sep 26, 2024

View reviewed changes

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

Merged via the queue into operator-framework:main with commit d9cf2ed Sep 26, 2024
16 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Use creds if present for pulling bundle images #1303

✨ Use creds if present for pulling bundle images #1303

anik120 commented Sep 24, 2024

netlify bot commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

anik120 Sep 26, 2024

joelanford Sep 26, 2024

anik120 Sep 26, 2024

everettraven Sep 26, 2024

anik120 Sep 26, 2024 •

edited

Loading

anik120 Sep 26, 2024

anik120 Sep 26, 2024

everettraven Sep 26, 2024

		logger.Error(err, "could not stat auth file path", "path", authFilePath)
		return ""

✨ Use creds if present for pulling bundle images #1303

✨ Use creds if present for pulling bundle images #1303

Conversation

anik120 commented Sep 24, 2024

Description

Reviewer Checklist

netlify bot commented Sep 24, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

anik120 Sep 26, 2024

Choose a reason for hiding this comment

joelanford Sep 26, 2024

Choose a reason for hiding this comment

anik120 Sep 26, 2024

Choose a reason for hiding this comment

everettraven Sep 26, 2024

Choose a reason for hiding this comment

anik120 Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

anik120 Sep 26, 2024

Choose a reason for hiding this comment

anik120 Sep 26, 2024

Choose a reason for hiding this comment

everettraven Sep 26, 2024

Choose a reason for hiding this comment

netlify bot commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

anik120 Sep 26, 2024 •

edited

Loading