Skip to content

✨ handle optionality of spec.grpcPodConfig.extractContent.cacheDir #3556

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

✨ handle optionality of spec.grpcPodConfig.extractContent.cacheDir #3556

wants to merge 1 commit into from

Conversation

grokspawn
Copy link
Contributor

Description of the change:
Allows grpcPodConfig.extractContent.cacheDir to be optional, in case folks are

  1. using custom catalog images w/o an existing opm serve cache; or
  2. wish to force re-generation of catalog cache based on copied catalog content

Motivation for the change:
To reduce the size of catalogsource images, we'd like to be able to provide OCI images which contain neither binary nor prebuilt cache. With improvements in cache-build pod startup, we should be able to operate without a pre-existing cache.

Architectural changes:

Testing remarks:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Bug fixes are accompanied by regression test(s)
  • e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
  • tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
  • Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive
  • Tests marked as [FLAKE] are truly flaky and have an issue
  • Code is properly formatted

@openshift-ci 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 Apr 10, 2025
@openshift-ci openshift-ci bot requested review from dtfranz and kevinrizza April 10, 2025 19:05
@grokspawn
Copy link
Contributor Author

grokspawn commented Apr 10, 2025 •
edited
Loading

Needs a PR in API to make this field optional before this will be functional, so hold this until that has merged.

Depends on operator-framework/api#421

@grokspawn grokspawn force-pushed the extractcontent-optional-cache branch from 8c68f2a to c2808f6 Compare April 11, 2025 16:05
@grokspawn grokspawn mentioned this pull request Apr 18, 2025
Merged
@grokspawn
Copy link
Contributor Author

Resolves /issues/3257

@grokspawn grokspawn force-pushed the extractcontent-optional-cache branch from c2808f6 to 2e113e9 Compare April 24, 2025 20:56
@grokspawn grokspawn changed the title WIP: handle optionality of spec.grpcPodConfig.extractContent.cacheDir 🌱 handle optionality of spec.grpcPodConfig.extractContent.cacheDir Apr 24, 2025
@openshift-ci 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 Apr 24, 2025
@grokspawn grokspawn force-pushed the extractcontent-optional-cache branch from 2e113e9 to 6c1a241 Compare April 25, 2025 15:09
camilamacedo86
camilamacedo86 reviewed Apr 28, 2025
View reviewed changes
pkg/controller/registry/reconciler/reconciler.go
// ExtractContent.CacheDir is optional, so we only add it if it is set
var extractArgs = []string{
"--catalog.from=" + grpcPodConfig.ExtractContent.CatalogDir,
"--catalog.to=" + fmt.Sprintf("%s/catalog", catalogPath),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we already have a check for this?

If ExtractContent is not empty and ExtractContent.CatalogDir is empty, we should raise an error with the following message:

"CatalogDir must be set when ExtractContent is provided."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have that because CatalogDir is listed as required

camilamacedo86
camilamacedo86 reviewed Apr 28, 2025
View reviewed changes
pkg/controller/registry/reconciler/reconciler.go
Comment on lines -291 to -292
"--catalog.from=" + grpcPodConfig.ExtractContent.CatalogDir,
"--catalog.to=" + fmt.Sprintf("%s/catalog", catalogPath),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This both we still have 👍

camilamacedo86
camilamacedo86 reviewed Apr 28, 2025
View reviewed changes
pkg/controller/registry/reconciler/reconciler.go
"--catalog.from=" + grpcPodConfig.ExtractContent.CatalogDir,
"--catalog.to=" + fmt.Sprintf("%s/catalog", catalogPath),
"--cache.from=" + grpcPodConfig.ExtractContent.CacheDir,
"--cache.to=" + fmt.Sprintf("%s/cache", catalogPath),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thsi both are under if grpcPodConfig.ExtractContent.CacheDir != "" { 👍

camilamacedo86
camilamacedo86 reviewed Apr 28, 2025
View reviewed changes
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @grokspawn

All shows fine for me here 👍
We just need rebase it with master
Then, all OK

Just a NIT: The emoji here I think should be ✨ OR 🐛 since 🌱 would be for case scenarios where has 0 impact in end users like ci fixes for example or bumps.

@joelanford joelanford changed the title 🌱 handle optionality of spec.grpcPodConfig.extractContent.cacheDir ✨ handle optionality of spec.grpcPodConfig.extractContent.cacheDir May 1, 2025
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2025
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 left review comments

@dtfranz dtfranz Awaiting requested review from dtfranz

@kevinrizza kevinrizza Awaiting requested review from kevinrizza

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@grokspawn @openshift-merge-robot @camilamacedo86