-
Notifications
You must be signed in to change notification settings - Fork 53
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
✨ Add PullSecret controller to save pull secret data locally #1322
✨ Add PullSecret controller to save pull secret data locally #1322
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1322 +/- ##
==========================================
- Coverage 76.42% 74.99% -1.44%
==========================================
Files 41 42 +1
Lines 2431 2507 +76
==========================================
+ Hits 1858 1880 +22
- Misses 400 443 +43
- Partials 173 184 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
d2dea55
to
500da77
Compare
500da77
to
7e1b8f7
Compare
Where the PR stands so far:
More details about this file here
|
fixes #1015 |
7e1b8f7
to
bb1966c
Compare
bb1966c
to
b5a5d25
Compare
cmd/manager/main.go
Outdated
@@ -101,6 +105,7 @@ func main() { | |||
flag.StringVar(&cachePath, "cache-path", "/var/cache", "The local directory path used for filesystem based caching") | |||
flag.BoolVar(&operatorControllerVersion, "version", false, "Prints operator-controller version information") | |||
flag.StringVar(&systemNamespace, "system-namespace", "", "Configures the namespace that gets used to deploy system resources.") | |||
flag.StringVar(&globalPullSecret, "global-pull-secret", "", "The namespace/name of the global pull secret that is going to be used to pull bundle images.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a thing to note, there is a flag.Var()
method that would allow us to implement a custom flag value with custom logic for handling how it is set, including validations.
I did something similar to this in kapp back when we were contributing preflight checks: https://github.com/carvel-dev/kapp/blob/ca6887d26d782636fe62a2b3f3bbbf5c91a965f3/pkg/kapp/preflight/registry.go#L64-L97
Might be overkill for this particular case, but this may be a good way to handle any custom flag validations we want in the future
aeadb18
to
d553087
Compare
60a4c92
to
ec18c90
Compare
ec18c90
to
6cdef28
Compare
6cdef28
to
3741d4a
Compare
3741d4a
to
bac538c
Compare
cacheOptions.ByObject[&corev1.Secret{}] = crcache.ByObject{ | ||
Namespaces: map[string]crcache.Config{ | ||
globalPullSecretKey.Namespace: { | ||
LabelSelector: k8slabels.Everything(), | ||
FieldSelector: fields.SelectorFromSet(map[string]string{ | ||
"metadata.name": globalPullSecretKey.Name, | ||
}), | ||
}, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also override the configuration for secrets in the systemNamespace
? IIUC we want to make sure we have access to all the release secrets that get created by the secret driver we use for partitioning helm release secrets for larger bundles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved pending an answer to this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dirty secret: iirc, the release secret driver is still a live client. So checking whether that is working won't actually tell you anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I don't actually see any configuration for secretes in the systemNamespace
. There's configuration for &ocv1alpha1.ClusterExtension{}
and &catalogd.ClusterCatalog{}
, but nothing for Secrets.
Dismissing because I had a question I forgot about that should be verified
if systemNamespace == "" { | ||
systemNamespace = podNamespace() | ||
} | ||
|
||
setupLog.Info("set up manager") | ||
cacheOptions := crcache.Options{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: If you can add some comments around what the below code doing it will add a lot to the readability of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced any further documentation is necessary. The crcache.Options
struct is pretty well documented. Once the reader understands what the controller-runtime cache does and what Options
can be used to configure, the code reads "configuring caching options for the objects we own + additional object Secret in a specific namespace".
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
bac538c
to
018510e
Compare
* ✨ Add PullSecret controller to save pull secret data locally RFC: https://docs.google.com/document/d/1BXD6kj5zXHcGiqvJOikU2xs8kV26TPnzEKp6n7TKD4M/edit#heading=h.x3tfh25grvnv * main.go: improved cache configuration for watching pull secret Signed-off-by: Joe Lanford <joe.lanford@gmail.com> --------- Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
f35edf6
RFC: https://docs.google.com/document/d/1BXD6kj5zXHcGiqvJOikU2xs8kV26TPnzEKp6n7TKD4M/edit#heading=h.x3tfh25grvnv
Description
Reviewer Checklist