Skip to content

Conversation

@trthomps
Copy link

@trthomps trthomps commented Nov 5, 2025

  • This MR adds support for google cloud storage via gsutil or gcloud storage

@trthomps trthomps requested a review from a team as a code owner November 5, 2025 20:31

gcs_exists() {
local key="$1"
local full_path="gs://${BUILDKITE_PLUGIN_GCS_CACHE_BUCKET}/$(build_key "${key}")"
Copy link
Contributor

@JoeColeman95 JoeColeman95 Nov 11, 2025

Choose a reason for hiding this comment

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

Fixes a spellchecker issue

Suggested change
local full_path="gs://${BUILDKITE_PLUGIN_GCS_CACHE_BUCKET}/$(build_key "${key}")"
local full_path
full_path="gs://${BUILDKITE_PLUGIN_GCS_CACHE_BUCKET}/$(build_key "${key}")"

local to="$1"
local from="$2"
local use_rsync='true'
local key="$(build_key "${to}")"
Copy link
Contributor

@JoeColeman95 JoeColeman95 Nov 11, 2025

Choose a reason for hiding this comment

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

Fixes a spellchecker issue

Suggested change
local key="$(build_key "${to}")"
local key
key="$(build_key "${to}")"

local from="$1"
local to="$2"
local use_rsync='false'
local key="$(build_key "${from}")"
Copy link
Contributor

@JoeColeman95 JoeColeman95 Nov 11, 2025

Choose a reason for hiding this comment

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

Fixes a spellchecker issue

Suggested change
local key="$(build_key "${from}")"
local key
key="$(build_key "${from}")"

Copy link
Contributor

@JoeColeman95 JoeColeman95 left a comment

Choose a reason for hiding this comment

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

Hey @trthomps

Thanks for the contribution! This is great 🥇

A couple of asks, if possible. Could you please update from main? We currently have a check failing due to some references being outdated.

Also, I can see from the bats tests that you added that 7/10 of them are failing. Please use buildkite-plugin-tester as this allows you to run the same tests we do on our end, and should take out a bit of the guess-work. Here's the breakdown of the fails:

✗ Quiet flag passed when environment is set
✗ File exists and can be restored after save  
✗ Folder exists and can be restored after save
✗ Prefix is used when environment is set
✗ gcloud storage CLI works when selected
✗ gcloud storage quiet flag passed when environment is set
✗ gcloud storage file operations work

Besides that, we have a couple of spell check warnings that were flagged as error handling issues. This due to way the local variables were declared – I've suggested a few fixes here to get you unblocked there.

Following on from the local vars, we have backends/cache_gcs ending without a new line, could you add an empty line at the end of the file, please?

In terms of logic, I've caught a couple of issues that are a cause for concern - I've highlighted that on the relevant lines.

Really appreciate the PR, and the time and effort you put in here – thanks for making Buildkite better for everyone 💪

# Check if the object exists using ls
# For directories, ls will list contents
# For files, it will return the file path
if gcs_cmd ls "${full_path}*" &>/dev/null; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible for us to do this without the wildcard? As this may cause some false positives

local full_path="gs://${BUILDKITE_PLUGIN_GCS_CACHE_BUCKET}/${key}"

# Check if it's a directory by trying to list it as a prefix
if gcs_cmd ls "${full_path}/" &>/dev/null; then
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be preferable here to use ls -d here if it's gsutil or the --max-results=1 flag here if it's gcloud as this could be a little fragile

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants