Skip to content

Conversation

@KastenMike
Copy link
Contributor

@KastenMike KastenMike commented Oct 1, 2024

Extending a blob requires full control. On the newer client styles (e.g. option.WithCredentialsJSON(j)), this is the default if no scope is specified.

Before:

gcs_immu_test.go:90:
        	Error Trace:	/Users/admin/repo/kopia/repo/blob/gcs/gcs_immu_test.go:90
        	Error:      	Received unexpected error:
        	            	googleapi: Error 403: Access denied., forbidden
        	            	unable to extend retention period to 2024-10-01 20:02:11 +0000 UTC

After:

PASS
ok  	github.com/kopia/kopia/repo/blob/gcs	3.176s

@codecov
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.23%. Comparing base (cb455c6) to head (1218dff).
Report is 310 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4151      +/-   ##
==========================================
+ Coverage   75.86%   77.23%   +1.36%     
==========================================
  Files         470      484      +14     
  Lines       37301    28867    -8434     
==========================================
- Hits        28299    22294    -6005     
+ Misses       7071     4667    -2404     
+ Partials     1931     1906      -25     

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

Copy link
Collaborator

@julio-lopez julio-lopez left a comment

Choose a reason for hiding this comment

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

Hold on.

@kreare
Copy link

kreare commented Oct 16, 2024

any update?

@KastenMike
Copy link
Contributor Author

KastenMike commented Oct 16, 2024

@jkowalski I see you added the generic readonly a few years ago and also the GCS specific read-only (in the original commit)

What do you think about just removing the scope variation completely? Removing it has 3 benefits:

  1. unified approach amongst the providers (s3/az/gcs) => use kopia readonly
  2. simplified client creation code in GCS
  3. simplified params for GCS, having 2 readonly flags is confusing imo

Then GCS would use the client default (ScopeFullControl) and the generic readonly.NewWrapper if the flag is set

@julio-lopez julio-lopez requested a review from jkowalski October 16, 2024 17:19
@julio-lopez julio-lopez dismissed their stale review October 16, 2024 17:21

Requesting @jkowalki's input

Copy link
Collaborator

@julio-lopez julio-lopez left a comment

Choose a reason for hiding this comment

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

+1: LG 👍

We should get @jkowalski input before merging.

@julio-lopez julio-lopez changed the title fix(repository): Allow extending blob retentions fix(repository): allow extending blob retentions Oct 22, 2024
Copy link
Contributor

@jkowalski jkowalski left a comment

Choose a reason for hiding this comment

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

LGTM

@jkowalski
Copy link
Contributor

Thanks for the PR!

@jkowalski jkowalski merged commit e20ec3d into kopia:master Oct 24, 2024
@julio-lopez julio-lopez deleted the minimal-gcs-fix branch October 25, 2024 00:11
alvistar pushed a commit to alvistar/kopia that referenced this pull request Oct 29, 2024
mcamou pushed a commit to mcamou/kopia that referenced this pull request Oct 30, 2024
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.

4 participants