Skip to content

cli: rely on the existing use_s3_caching option to enable caching #223

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

Merged
chouquette merged 4 commits into from
Mar 7, 2025
Merged

cli: rely on the existing use_s3_caching option to enable caching #223

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d712807
cli: rely on the existing use_s3_caching option to enable caching
chouquette Mar 6, 2025
904d7d2
cli: remove now unused populate_s3_cache option
chouquette Mar 6, 2025
80dabb6
don't attempt to populate cache when we only have read access
chouquette Mar 7, 2025
1386d59
add a comment to clarify the logic
chouquette Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions lib/omnibus/cli.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,6 @@ def execute!
desc: "Use the given manifest when downloading software sources.",
type: :string,
default: nil
method_option :populate_s3_cache,
desc: "Populate the S3 cache.",
type: :boolean,
default: false
desc "build PROJECT", "Build the given Omnibus project"
def build(name)
manifest = if @options[:use_manifest]
Expand All @@ -85,8 +81,11 @@ def build(name)
project = Project.load(name, manifest)
say("Building #{project.name} #{project.build_version}...")
# Not sure why we need the !Omnibus::S3Cache.fetch_missing.empty? check here
Omnibus::S3Cache.populate if @options[:populate_s3_cache] && !Omnibus::S3Cache.fetch_missing.empty?
Omnibus::S3LicenseCache.populate if @options[:populate_s3_cache]
# We only want to populate the cache when we actually can. In some cases, we're only reading anonymously
# from the cache bucket (mostly when the build isn't ran on gitlab). When that is the case, we want to disable
# the cache population since it would fail.
Omnibus::S3Cache.populate if Omnibus::Config.use_s3_caching && Omnibus::Config.s3_authenticated_download && !Omnibus::S3Cache.fetch_missing.empty?
Copy link

@alopezz alopezz Mar 7, 2025

Choose a reason for hiding this comment

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

Why Omnibus::Config.s3_authenticated_download?

Copy link
Author

@chouquette chouquette Mar 7, 2025

Choose a reason for hiding this comment

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

Good question, I'll add a comment

Copy link
Author

@chouquette chouquette Mar 7, 2025

Choose a reason for hiding this comment

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

Done. It turns out that S3_OMNIBUS_CACHE_BUCKET being defined is not enough to enable filling the cache.
Our macOS builds are reading anonymously from the bucket, but can't write to it, which is why there was a different option.
I'm not sure if it's worth keeping a dedicated flag for that since we already have enough information in the environment, but that feels a bit brittle

Copy link

@alopezz alopezz Mar 7, 2025

Choose a reason for hiding this comment

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

I see; that's interesting, I hadn't thought that we might be using the cache like that. Which, by the way, does feel a bit brittle, does that mean that we have a hidden dependency from the macOS build to the rest of builds then?

Copy link
Author

@chouquette chouquette Mar 7, 2025

Choose a reason for hiding this comment

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

Yes, I guess if there was a macOS specific dependency we would have the same issue we had on Windows yesterday.
Hopefully we'll move our macOS builds to gitlab soon enough and we can just remove this workaround.
I'll create a card for it

Omnibus::S3LicenseCache.populate if Omnibus::Config.use_s3_caching && Omnibus::Config.s3_authenticated_download
project.download
project.build

Expand Down