Skip to content
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

fix: bucket prefix #3808

Merged
merged 10 commits into from
Jan 17, 2025
Merged

fix: bucket prefix #3808

merged 10 commits into from
Jan 17, 2025

Conversation

markphelps
Copy link
Collaborator

Should fix: #3807

Re: #2589

It seems gcp was adding the prefix twice, so it would make requests like:

https://storage.googleapis.com/flipt-test/productionproduction.flipt.yml

I assume this is because of us setting both the query param prefix when building the bucket URL and by setting prefix+key for all object requests.

I discovered that Go Cloud has support for prefixed buckets naturally: https://gocloud.dev/howto/blob/#prefix

Which now does the right thing and lists the contents at:

https://storage.googleapis.com/flipt-test/production/flipt.yml

I still need to test if this breaks S3 prefix though

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
* main:
  fix: update flag with no metadata (#3791)
  chore(deps): bump github.com/jackc/pgx/v5 from 5.7.1 to 5.7.2 (#3786)
  chore(deps): bump go.opentelemetry.io/otel/exporters/prometheus (#3785)
  chore(deps): bump github.com/go-git/go-git/v5 from 5.13.0 to 5.13.1 (#3783)
  chore(deps): bump golang.org/x/oauth2 from 0.24.0 to 0.25.0 (#3784)
  chore(deps): bump github.com/aws/aws-sdk-go-v2/service/ecrpublic (#3787)
  chore(deps): bump class-variance-authority from 0.7.0 to 0.7.1 in /ui (#3782)
  chore(deps-dev): bump @types/node from 18.19.65 to 18.19.69 in /ui (#3780)
  chore(deps): bump @uiw/codemirror-theme-tokyo-night in /ui (#3779)
  chore(deps): bump react-chartjs-2 from 5.2.0 to 5.3.0 in /ui (#3778)
  chore(deps): bump next in /examples/nextjs/pages-router (#3776)
  chore(deps): bump next in /examples/nextjs/app-router (#3777)
  chore: add release-notes to gitignore
  chore: fix release notes build
  chore: prep 1.54 for release (#3775)
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
* 'main' of https://github.com/flipt-io/flipt:
  chore(deps): bump @fortawesome/fontawesome-svg-core in /ui (#3802)
  chore(deps): bump github.com/spf13/afero from 1.11.0 to 1.12.0 (#3799)
  chore(deps): bump golang.org/x/net from 0.33.0 to 0.34.0 (#3800)
  chore(deps): bump go.opentelemetry.io/otel/exporters/zipkin (#3796)
  chore(deps): bump cloud.google.com/go/storage from 1.49.0 to 1.50.0 (#3798)
  chore(deps): bump go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp (#3797)
  chore(deps): bump alpine from 3.21.0 to 3.21.2 in /build (#3795)
  chore(deps): bump lucide-react from 0.468.0 to 0.471.0 in /ui (#3801)
  chore(deps-dev): bump @types/node from 18.19.69 to 18.19.70 in /ui (#3803)
  chore(deps): bump @fortawesome/free-brands-svg-icons in /ui (#3804)
  chore(deps): bump @codemirror/state from 6.5.0 to 6.5.1 in /ui (#3805)
  chore(deps): bump golang.org/x/tools from 0.28.0 to 0.29.0 in /_tools (#3793)
  chore(deps): bump google.golang.org/protobuf in /_tools (#3794)
  chore(deps-dev): bump eslint-plugin-react from 7.37.2 to 7.37.3 in /ui (#3781)
  chore: Add back release footer (#3792)
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
@markphelps markphelps requested a review from a team as a code owner January 15, 2025 22:09
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
@erka
Copy link
Collaborator

erka commented Jan 15, 2025

Prefix is a tricky thing. Prefix is not a folder as object storages doesn't have folders. At least in the past... AWS has changed this a bit recently. Prefixes production and production/ are not the same thing. Just for the record.

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 33.33333% with 8 lines in your changes missing coverage. Please review.

Project coverage is 64.80%. Comparing base (31babb4) to head (910e6da).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/storage/fs/store/store.go 0.00% 7 Missing ⚠️
internal/storage/fs/object/store.go 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3808      +/-   ##
==========================================
- Coverage   64.84%   64.80%   -0.05%     
==========================================
  Files         171      171              
  Lines       17391    17387       -4     
==========================================
- Hits        11277    11267      -10     
- Misses       5418     5423       +5     
- Partials      696      697       +1     
Flag Coverage Δ
unittests 64.80% <33.33%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
@markphelps
Copy link
Collaborator Author

Prefix is a tricky thing. Prefix is not a folder as object storages doesn't have folders. At least in the past... AWS has changed this a bit recently. Prefixes production and production/ are not the same thing. Just for the record.

I think the go cloud blob.PrefixedBucket should handle these cases no? At the very least all the ITs now pass again. Mind giving it a review @erka ?

@markphelps markphelps requested a review from GeorgeMac January 16, 2025 16:20
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
@markphelps markphelps requested a review from erka January 17, 2025 15:45
Copy link
Collaborator

@erka erka left a comment

Choose a reason for hiding this comment

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

looks good

@markphelps markphelps added the automerge Used by Kodiak bot to automerge PRs label Jan 17, 2025
@kodiakhq kodiakhq bot merged commit cb278c8 into main Jan 17, 2025
38 of 40 checks passed
@kodiakhq kodiakhq bot deleted the fix-bucket-prefix branch January 17, 2025 15:50
markphelps added a commit that referenced this pull request Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Used by Kodiak bot to automerge PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server Unable to Locate .flipt.yml and features.yml in GCloud Bucket
2 participants