This repository was archived by the owner on Mar 3, 2025. It is now read-only.
✨ Add PullSecret controller to save pull secret data locally#425
Merged
Conversation
6119a87 to
8375843
Compare
Member
Author
|
Test performed: Created:
Controller logs: Log line to note from the Unpacker: Final result 🎉 : |
8375843 to
2605514
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #425 +/- ##
==========================================
+ Coverage 35.28% 37.31% +2.02%
==========================================
Files 14 15 +1
Lines 802 922 +120
==========================================
+ Hits 283 344 +61
- Misses 472 529 +57
- Partials 47 49 +2 ☔ View full report in Codecov by Sentry. |
skattoju
reviewed
Oct 4, 2024
| err := r.Get(ctx, req.NamespacedName, secret) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| logger.Info("secret not found") |
There was a problem hiding this comment.
should this be a warn or error saying it will be unable to pull from pvt registries ?
Member
Author
There was a problem hiding this comment.
So there's no WARN for this particular logger we're using, only Info or Error. I don't think this needs to be an Error, especially since the function we're calling logs additional messages about the auth file being deleted.
skattoju
reviewed
Oct 4, 2024
2605514 to
d8159ec
Compare
trgeiger
reviewed
Oct 4, 2024
| flag.StringVar(&keyFile, "tls-key", "", "The key file used for serving catalog contents over HTTPS. Requires tls-cert.") | ||
| flag.IntVar(&webhookPort, "webhook-server-port", 9443, "The port that the mutating webhook server serves at.") | ||
| flag.StringVar(&caCertDir, "ca-certs-dir", "", "The directory of CA certificate to use for verifying HTTPS connections to image registries.") | ||
| flag.StringVar(&globalPullSecret, "global-pull-secret", "", "The namespace/name of the global pull secret that is going to be used to pull bundle images.") |
Contributor
There was a problem hiding this comment.
I think this could be a bit confusing at a glance. One could read this as the namespace OR the name. Could format it like the error message below "/"
trgeiger
reviewed
Oct 4, 2024
| // https://github.com/containers/image/blob/main/docs/containers-auth.json.5.md | ||
| err := os.WriteFile(r.AuthFilePath, dockerConfigJSON, 0600) | ||
| if err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to write secret data to file: %w", err) |
Contributor
There was a problem hiding this comment.
similar comment here as Sid's below re: revealing file location
Member
Author
There was a problem hiding this comment.
I removed the other ones, but I have the urge to keep this one. We'd want to see the specific error message for this when we get a must-gather (logs) for a bug report related to this.
trgeiger
reviewed
Oct 4, 2024
| return nil, reconcile.TerminalError(fmt.Errorf("error parsing catalog, catalog %s has a nil image source", catalog.Name)) | ||
| } | ||
|
|
||
| srcCtx, err := i.SourceContextFunc(l) |
Contributor
There was a problem hiding this comment.
linter pointed this out, but no err check here
Member
Author
There was a problem hiding this comment.
Oops missed this, thanks for pointing it out. Added an error check below.
71bb7a0 to
ad9c5b0
Compare
tmshort
reviewed
Oct 7, 2024
| storageDir = "catalogs" | ||
| authFilePath = "/etc/catalogd/auth.json" | ||
| storageDir = "catalogs" | ||
| authFilePrefix = "catalogd-global-pull-secret.json" |
Contributor
There was a problem hiding this comment.
Should this have the .json extension?
Member
Author
There was a problem hiding this comment.
Ah I see the confusion. It says "prefix" there but it's actually the suffix. Changing it. Also I just noticed the fmt.Sprintf already has the .json, thanks for pointing it out.
Member
Author
There was a problem hiding this comment.
Should be fixed now. Here's what it looks like https://go.dev/play/p/FJlQgd_KS5R
ad9c5b0 to
c1bf01a
Compare
Contributor
|
Should this have changes that match operator-framework/operator-controller#1322 ? |
Member
Author
@tmshort do you mean this? Am I missing any changes? I've been trying to keep both of them in sync and I don't see anything that I missed. |
everettraven
previously approved these changes
Oct 8, 2024
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
c1bf01a to
2427c8a
Compare
everettraven
approved these changes
Oct 8, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RFC: https://docs.google.com/document/d/1BXD6kj5zXHcGiqvJOikU2xs8kV26TPnzEKp6n7TKD4M/edit#heading=h.x3tfh25grvnv